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

Format project using Prettier #802

Merged
merged 8 commits into from
Jan 2, 2024
Merged

Format project using Prettier #802

merged 8 commits into from
Jan 2, 2024

Conversation

withinfocus
Copy link

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.

@jsamuel1
Copy link

jsamuel1 commented Dec 16, 2023 via email

@marcone
Copy link
Owner

marcone commented Dec 17, 2023

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.
That could probably be addressed, but I don't really see the value of doing a one-time reformatting, and I don't like the idea of adding additional dependencies on prettier/npm to run it in some automated fashion.
I'll think about it some more, but chances are I won't merge this.

@withinfocus
Copy link
Author

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.

@marcone
Copy link
Owner

marcone commented Dec 17, 2023

I pretty much just use vi/vim, usually on the Pi itself.
What are the html errors that it found? (I can't tell from the diff because of the way github displays it)

@withinfocus
Copy link
Author

You can see this by running prettier --write "**/*.*" --ignore-unknown.

Errors:

  • teslausb-www/html/index.html: SyntaxError: Unexpected character "<" (878:3) -- missing >
  • teslausb-www/html/index.html: SyntaxError: Unexpected closing tag "div". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags (847:13) -- canvas tag needed closing

This isn't really Prettier's intention, but it couldn't even work the document until I fixed those two.

@marcone
Copy link
Owner

marcone commented Dec 18, 2023

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.
Does Prettier have a validate-only mode that could be used as a presubmit-checker, similar to how shellcheck is currently used for the bash scripts?

@withinfocus
Copy link
Author

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.

@withinfocus
Copy link
Author

@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.

@marcone
Copy link
Owner

marcone commented Dec 23, 2023

I'm OK with changing the .yml and .md files, but don't want to change the html/css at this time.

@withinfocus
Copy link
Author

@marcone done. I added a Prettier ignore so this is avoided in the future too if it comes up, per your preferences.

Copy link
Owner

@marcone marcone left a 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.

doc/OneStepSetup.md Outdated Show resolved Hide resolved
doc/SetupRClone.md Outdated Show resolved Hide resolved
@withinfocus
Copy link
Author

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.

@marcone
Copy link
Owner

marcone commented Dec 26, 2023

Is Prettier aware of "Github flavored markdown", or does it only deal with standard markdown ?

@withinfocus
Copy link
Author

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.

@marcone marcone merged commit 02f4f57 into marcone:main-dev Jan 2, 2024
1 check passed
@withinfocus withinfocus deleted the formatting branch January 2, 2024 19:04
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