-
Notifications
You must be signed in to change notification settings - Fork 360
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
Format project using Prettier #802
Conversation
I am on the fence about this, but leaning towards "probably not worth the effort/hassle". The markdown and yml changes look OK, but I definitely don't like what it did to the html files. There's too much indent for my taste now. I want the embedded css and javascript to start at start-of-line, not have 6-space indent. |
Per the other comment I think it'd be great to provide a format configuration. What editor do you use for this project? A great deal ship with Prettier support built-in these days and while it's a one-time change, by driving this with configuration it will be maintained and technical debt eliminated. A good example -- while it can't be easily seen given the many other formatting changes -- is that Prettier found three HTML errors that I corrected. I don't really have any opinions on how it should be configured and if you have a small list of desires such as indentation rules they can be applied. Prettier's default interpretations are backed by a significant amount of contributors' feedback, best practices, and standards. |
I pretty much just use vi/vim, usually on the Pi itself. |
You can see this by running Errors:
This isn't really Prettier's intention, but it couldn't even work the document until I fixed those two. |
Thanks, I ran index.html through the W3C html validator and it found a few more issues that Prettier did not. Not surprising I suppose, since Prettier isn't an html validator. |
Prettier can be used as an informative tool yes, but it's meant to be applied vs. just validate. Expanding your lint process can actually get pretty exciting. My recommendation is to use https://megalinter.io/latest/ as a GitHub Action. Perhaps overkill, but once set up it does its work and can even automatically apply changes. It includes shellcheck and a bunch of others that will be quality of life improvements. I use it all over the place and https://github.com/withinfocus/withinfocus.github.io/blob/develop/.github/workflows/lint.yml is one example. An alternative is https://github.com/super-linter/super-linter but I moved away from it since Mega-Linter is more powerful. |
@marcone any further thoughts on this? I'd love to get this one-time change merged and then I'll rebase my Pi5 PR. The underlying EEPROM changes appear to be resolved and I'd like to finish testing on my device and add this support. Some of the philosophy behind a change like this is available at https://prettier.io/docs/en/why-prettier. |
I'm OK with changing the .yml and .md files, but don't want to change the html/css at this time. |
@marcone done. I added a Prettier ignore so this is avoided in the future too if it comes up, per your preferences. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's an easy fix to make Prettier not mangle some of the .md files, but if not, it might be better to exclude those too, and limit this change to just the files where Github's "rich diff" shows no changes.
I did some visual checks but let me really go through these. The issue is content that isn't allowed as-is in Markdown to begin and I'd rather just fix them up. |
Is Prettier aware of "Github flavored markdown", or does it only deal with standard markdown ? |
There isn't anything that would get in the way with Prettier for GitHub's unique additions and it's really about the rendering. I reviewed all the changes again and believe the couple strange changes are fixed up now. Sometimes all it takes is an errant or unclosed block to make the changes cascade. I applied changes consistently where I saw a need such as sequential numbering or use of the "note" block so that it's uniform. |
As I began some small work on this project I noticed that Prettier could have a run through the codebase and grab a few changes based on configured defaults and some best practices. This does have some opinions of Prettier applied when configuration otherwise wasn't present, but thousands of projects apply Prettier regularly and I think it can be acceptable here.
Prettier also found a couple syntax issues in the web experience.