-
Notifications
You must be signed in to change notification settings - Fork 577
use prettier to format all of js,json files #1883
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
Conversation
|
Nice work. I'll check it out ASAP and report back. |
crunchtime-ali
left a comment
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.
ok, thank you for clarifying.
Besides that the PR LGTM.
ricardograca
left a comment
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.
Finally, if you could change bracketSpacing to false I think it would help to cut down the number of lines touched by this PR.
Still, I'm not entirely convinced I prefer this over Standard.js, so can you explain its advantages and what the development workflow looks like with this? Do I have to install any IDE plugins? Run a script before committing?
.prettierrc
Outdated
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'd prefer if this was set to "none". Also, can you set semi: false?
lib/base/collection.js
Outdated
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.
Wow. I really hate how the function arguments are split into individual lines like this. Makes no sense for such a small number of arguments. Is it possible to avoid this?
lib/base/events.js
Outdated
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.
This is weird. Splitting a single line fat arrow function into multiple lines makes its intention a bit ambiguous: e.g. was it supposed to be returning something or was it supposed to return undefined? Did the author made a mistake?
lib/base/model.js
Outdated
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.
This is definitely super weird 😕 Why is the options.patch in a single line by itself? Is there like a 80 column max line length limit or something?
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.
Yes, it wraps per default at 80 columns however you can set it via printWidth to a more reasonable amount. I suggest 120 columns. This should fix most of your aforementioned problems. How many columns do you suggest @ricardograca
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.
👍 120 sounds good to me as well. 80 is definitely too narrow.
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.
@chentsulin Can you please set printWidth: 120 and run prettier on the codebase again?
c66d786 to
e17aea3
Compare
|
Added those config: |
|
@chentsulin Can you address the last remaining comment above? I can do that after merging this, but I'd like to avoid several multi-thousand lines PRs if possible. |
|
I set Before: After |
|
Many thanks! |


Introduction
Use prettier and husky + lint-staged precommit hook to auto format all of js, json files
Proposed solution
Follow @tgriesser's
.prettierrcconfig and npm scripts in knex/knex#2697