Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSS formatter #699

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Add CSS formatter #699

merged 3 commits into from
Apr 9, 2024

Conversation

lavigneer
Copy link
Contributor

@lavigneer lavigneer commented Apr 9, 2024

Add CSS Formatter

Description

Hi there! First off I'd like to say I think this project is really neat, I like how you're leveraging existing treesitter grammars to do code formatting. I was hoping to help contribute to this project as I'd love to see it grow and I thought adding a CSS formatter would be not overly challenging but useful.

I wanted to build as close to an approximation in terms of formatting is Prettier, which is the go to formatter in the web ecosystem at the moment. I first built a script that scraped all the examples from https://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/full/flat/index.html and put them in the test css files that I ran my queries against. My original plan was to commit that as the test case but I realized a real-world example would be more useful and in fact there were some things missing in those w3 examples, like animation keyframes and charset to name a couple.

I noticed you use bootstrap in your website pacakge, so I opted instead to pull the minified version of the latest bootstrap utilities build and run it through my formatter to get the expected result. To ensure that the formatter I added was in fact doing what it should have, I also ran the same minified bootstrap css through prettier and ran a diff against my format and theirs. The only differences were changes to the actual content in some places by prettier. Specifically it added a 0 before any decimal numeric that was not greater than 1 (e.g., .75 -> 0.75) and removing quotes around the data-bs-theme value selectors. See
prettier-diff.txt for the entire diff.

Please let me know if there's anything I could do better or if you're not accepting PRs for new language support at this time, I certainly understand and can close this off.

Thanks for your time.


P.S. there were a few things I noticed while trying to work on this that may be useful information:

  1. There doesn't seem to be a way to have something for max line width. Perhaps leveraging the softline captures to tell the formatter that it's ok to wrap long lines of code across multiple lines without requiring the developer to multiline first?
  2. There is no way to force newlines between items. You can allow newlines above something with allow_blank_line_before, but in some cases it could be useful to not only allow it but enforce it?
  3. The nix develop command appears to be in a broken state right now. I spent some time debugging it and it appears that the revision of tree-sitter-bash that you are pinned to references a revision of a submodule for https://git.savannah.gnu.org/git/bash.git that does not exist. The specific error I get is Cannot find Git revision 'a99d905216cc0aac5de0c3050f4afc54e21c6bc5' in ref 'refs/heads/master' of repository 'https://git.savannah.gnu.org/git/bash.git'!

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @lavigneer! This is great work. Thank you 🙇

@ErinvanderVeen and I looked this over, together, this morning and it's definitely merge-worthy. Thank you again for the effort. Beyond that, let me try to address the points you raise:

  1. There doesn't seem to be a way to have something for max line width. Perhaps leveraging the softline captures to tell the formatter that it's ok to wrap long lines of code across multiple lines without requiring the developer to multiline first?

That's right; Topiary has no concept of line-length and using that as a constraint to force reflowing, like other formatters do. I'm not sure if this is "by design" or "by accident", but the general nature of Topiary means that it's not obvious how to wrap long lines in all cases.

  1. There is no way to force newlines between items. You can allow newlines above something with allow_blank_line_before, but in some cases it could be useful to not only allow it but enforce it?

allow_blank_line_before is relative to the input, rather than the output (i.e., how blank lines are added by Topiary). So, if there are no blank lines between allowed elements in the input, any added will get eaten up by the post-processor. This is, admittedly, somewhat unexpected behaviour and we'll look into correcting this 👍

  1. The nix develop command appears to be in a broken state right now. I spent some time debugging it and it appears that the revision of tree-sitter-bash that you are pinned to references a revision of a submodule for https://git.savannah.gnu.org/git/bash.git that does not exist. The specific error I get is Cannot find Git revision 'a99d905216cc0aac5de0c3050f4afc54e21c6bc5' in ref 'refs/heads/master' of repository 'https://git.savannah.gnu.org/git/bash.git'!

Thanks for raising this. It's happened before and we'll look into it 👍

@ErinvanderVeen ErinvanderVeen merged commit 9217ebd into tweag:main Apr 9, 2024
1 check passed
@lavigneer
Copy link
Contributor Author

Thanks for taking the time to take a look at this and merging it in so quickly. I'll try to keep an eye on any issues that might come up with the css formatting and can lend a hand where desired.

@Xophmeister
Copy link
Member

@lavigneer We are planning on announcing your contribution, amongst many other things, on our blog. Are you happy for us to mention you by name?

@lavigneer
Copy link
Contributor Author

@Xophmeister that would be great, please feel free to do so 😄 Thanks for reaching out!

@Xophmeister
Copy link
Member

Thanks, @lavigneer 🙏 The post is due to be published next week (Thursday 22nd)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants