-
Notifications
You must be signed in to change notification settings - Fork 637
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
introduces prettier for automatic code formatting #560
Conversation
If I would be paranoid, I would actually just take in the code for setting up the prettier and run the formatter myself to be sure that there was no extra code changes slipped in along the formatting changes :) No offense meant to @capaj I'm just generally not trusting anyone in the net 😬 |
@elhigu definitely. I'd be paranoid as well. You can do very nasty things in a few obcuscated lines of JS. |
I'm happy to see this change, it will make digging into the source code easier 👍 . |
@oliviertassinari I agree that this is a good change because it enforces a single style but the changes seem pretty minimal to me. How does this make the code more readable? |
@koskimas I'm using prettier on all my projects. I'm simply used to the way it's formatting the source code, e.g. Material-UI. Hence, it's more intuitive to me. |
@capaj I have never used git hooks. Is there no way to add them to the repo so that they get installed to everyone that clone objection? I don't see the hooks in the PR (or am I just blind?). If there is no way to enforce the use of prettier, I think its usefulness is minimal. |
@koskimas prettier can be installed as a dev dependency, run on the CI and with a package script so everybody can just run it to make the CI pass. |
Git hooks get set up by husky once you npm install. Husky is quite handy like that. |
Also this is not just about readability. I see the main benefit in the automatic formatting. Every contributor can write code as they are used to and it gets automatically formatted. That's the most important part for me. |
I would advise against using the lint-staged script:
This can lead to bad commits when staging partial files (hunks). I just got bit by this in one my projects. I would instead suggest using the prettier plugins for eslint and a single |
@capaj I've been testing things with prettier and I generally like its style, but one thing really bugs me: prettier tries to put everything on one line when it can. This becomes ugly with chained method calls that objection uses a lot. For example: Person
.query()
.where('a', '<', 2)
.where('b', 4)
.eager('foo') Becomes Person.query().where('a', '<', 2).where('b', 4).eager('foo'); "new line per method call" style is better with github diffs too. You actually see the changed method call and not just that something in the chain changed. Changing |
Actually I was wrong. prettier does break the calls on separate lines if there are three or more method calls. I'd like it to do that after two calls, but maybe I can live with three. |
@koskimas I will check up on prettier team if they'd consider the maximum number of calls per line as a new config. I'd personally prefer at most 2 calls per line as well. |
@koskimas I agree, and felt the same way a couple months ago when I started using I have since chosen not to care, in deference to having everything be consistent and not worrying about line wrapping. @capaj I suggest you revert most of this PR, and only add the prettier config. @elhigu or @koskimas can then produce the subsequent diffs. |
@juanjoLenero's point about partial files is a good one. I use |
@@ -5,14 +5,21 @@ | |||
"main": "lib/objection.js", | |||
"license": "MIT", | |||
"scripts": { | |||
"precommit": "lint-staged", |
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.
if you want to add a "lint" script, that'd be nice, but I think precommit is going to be a PITA.
I'd +1 the following:
"prettier": "prettier --write",
"istanbul": "^0.4.5", | ||
"knex": "0.x", | ||
"lint-staged": "^4.2.3", |
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.
No thanks to adding lint-staged
and husky
deps, especially given @koskimas git-hunk commit flow.
I think will go with |
Done 17e58bb |
You murdered all your padded blocks; sorry but that is not more readable. I don't understand this cult of prettier. |
@kswope you have any better alternatives in mind? |
I just use jsbeautify, but that's just for the elderly it seems There's a thread here about using prettier but keeping the padded blocks I don't know why they refuse to add a padded blocks option, I feel like they are trolling everybody. |
I had never heard about that before, need to check it out. I haven't seen any js/ts auto formatter yet to whose output I would have been completely happy with. EDIT: Looks like I wouldn't be happy with jsbeautify either beautifier/js-beautify#1064 |
Anyways I don't think that this pull request is a good place to keep on discussion about different code formatters. |
codebase was formatted pretty nicely even before, but it helps not having to do it manually