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

Prettify #2185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Prettify #2185

wants to merge 2 commits into from

Conversation

Faithfinder
Copy link
Contributor

I've seen the dialog in #2157 about formatting and decided to see what happens if I run Prettier on the repo. This is the result.

Also added a .git-blame-ignore-revs file to exclude myself from git-blame. Unfortunately, there's not way to share git config with the repository, so everyone will need to run the command git config blame.ignoreRevsFile .git-blame-ignore-revs

Read more: https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame

@wbt
Copy link
Contributor

wbt commented Aug 31, 2022

Thanks for the effort!
The README.md file table of contents seems to have changed quite a lot - was that intended?
Reviewing through releases.md, I'm also not sure that all the changes adding/eliminating line breaks or parens (parens example: Readme:998) and case changes (e.g. Readme:1207) are really for the best, especially in documentation which we want to be easy to read.

@Faithfinder
Copy link
Contributor Author

Mmm... Table of contents changes were probably due to something gone wrong during merging.

Redid everything without including the docs.
I can't really control what prettier does - it just follows repo settings, so I think not touching the docs at all is the best decision.

Line breaks don't affect display though.

### Heading
 - List item

and

### Heading

 - List item

Are rendered the same. I think the idea behind these line breaks are improving source readability. Also, AFAIK, for some parser, not surrounding a heading by whitespace is an error (see markdownlint)

@wbt
Copy link
Contributor

wbt commented Aug 31, 2022

Thanks for the re-run, though I'm still not convinced I like what all it does.

For example, removing the close curly brace of the try block from the start of the catch line (see. e.g. examples/create-file.js) seems like poor practice that could open the door to separating those more than they should be separated - as a rule, I always keep those together. The addition of semicolons at the ends of lines generally makes sense, but in some places (like examples/custom-pretty-print.js:10 it seems like this would cause a dirtier diff when altering that pipeline. Removal of line breaks e.g. in examples/delete-level.js and examples/errors.js have a similar impact. The addition of line breaks later in that file seem to harm readability more than it helps, but not in a way that might be easy for a machine following simple rules to detect.

The repo settings are both fairly minimal and recent, and I don't have time now to go through each option and get it set right, nor even to go through a detailed review of several hundred changed lines modified by the application of simple rules that sometimes make things worse.

@Faithfinder
Copy link
Contributor Author

Prettier barely has settings though. At the end of the day, if you don't like what it does, it should be removed from the repo, otherwise it's just confusing for contributors.

It is the modern "Golden standard" though. I would accept all changes for standardization, even if I personally dislike something it does. And if something is really annoying, then you can tweak it and re-run on the repo.

You're the boss of course. I don't have real stake at this, after the #2181 is resolved I doubt I'll have anything to contribute any time soon

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.

2 participants