kabisa / maji

Maji is a framework to build great hybrid mobile apps.
https://www.majimobile.com/
MIT License
18 stars 10 forks source link

The "prettier" pre-commit hook makes "git add -p" useless #186

Closed ljpengelen closed 5 years ago

ljpengelen commented 6 years ago

When I want to commit part of a (JavaScript) file, the prettier pre-commit hook prettifies the whole file and commits the result.

ljpengelen commented 6 years ago

What value does prettier add? Isn't eslint enough to verify that everything is pretty before committing?

pascalw commented 6 years ago

Hm yes I'm afraid this is a limitation of this workflow and pretty difficult to solve. Instead of git adding prettified files we'd have to re-add the staged chunks, but those chunks might have changed due to formatting.

Perhaps you work around this by doing the following:

This is probably not ideal since you might get conflicts when you pop the stash since the file might have changed, but I can imagine it might work in many cases.

If not, you're of course free to disable the hook, or prettier completely if that suits you better.

pascalw commented 6 years ago

The value that prettier is adding is that you don't have to worry about formatting your code at all. You write your code however you want and prettier will make it look consistent.

Eslint is not (as much) about style as it's about detecting commons problems.

ljpengelen commented 6 years ago

If prettier has something to complain about, you'll also see that as part of eslint's output, because prettier runs as part of eslint as well, right? When you remove the last exit 0 from the eslint script that's part of the hook (I don't know why that statement is there in the first place), you won't be able to commit ugly code as well, regardless of whether prettier is included in the hook.

Those that don't want to manually format their code can run yarn prettier.

I'm removing prettier for the project I'm working on now and make the commit fail when eslint has complaints.

ruisalgado commented 6 years ago

indeed the way it's set up in the project template eslint already uses prettier under the hood as part of its set of rules. This makes the prettier pre-commit hook redundant.

I also stopped using it in my current project and instead use only eslint and eslint --fix.

ljpengelen commented 6 years ago

We should either document that you cannot blindly git add -p with Maji and think you'll get away with it or remove prettier from the hook. I'm willing to make a PR for either of the two options. Which one should it be?

ruisalgado commented 6 years ago

I think you need to do both currently (the eslint hook affects git add -p in the same way as the prettier hook)

ljpengelen commented 6 years ago

I don't understand that. The eslint hook doesn't alter what's staged as far as I can see.

ruisalgado commented 6 years ago

oh sorry you're right, I was looking at my current project where I changed it to behave like the prettier hook (does eslint --fix and stages changes). I guess this is just a personal preference though, I'm ambivalent to whether it should be the behavior that ships with maji

pascalw commented 6 years ago

I would propose to:

I think this is a sensible default because with this you don't have to think about formatting at all. I would hate for the default behavior to be that a manual prettier run is required before every commit.

ljpengelen commented 6 years ago

We could check whether there's overlap between git diff --name-only and git diff --cached --name-only. If there's overlap, we just lint and leave any issues for the user to resolve manually. If there's no overlap, we let eslint resolve issues.

ljpengelen commented 5 years ago

As of version 8.0.0, lint-staged supports partial commits: https://hackernoon.com/announcing-lint-staged-with-support-for-partially-staged-files-abc24a40d3ff