search

codeforboston / cliff-effects

Cliff effects guidance prototype (archived)
https://codeforboston.github.io/cliff-effects/#/
MIT License
30 stars 64 forks source link

pre-commit hooks: remove 'lint-staged' and carets? #908

Open knod opened 5 years ago

knod commented 5 years ago

About the awesome changes merged in #904.

  1. The 'lint-staged' package doesn't actually limit npm lint fixes to just staged files. It changes the files, though it doesn't add them. Considering this will be in everyone's code and all files will be linted when committed and there shouldn't be leftover files to lint, it shouldn't be a problem either way, but we might consider getting rid of 'lint-staged'.

  2. We'd agreed not to have carets in version numbers to try to stabilize our package-lock.json. Hasn't worked, but carats my confuse further efforts.

On other, less crucial notes, just so they're in the record:

  1. Right now it prints console logs when it does its thing, which might make newer folks nervous. There's currently no way to turn that off. Personally, I do generally go for explicit over implicit, but I'm not sure about this specific situation since it'll be happening right at the start. Depending on the outcome of the discussion, we could make a PR on husky's repo.

  2. It's strange how it can make a commit even if the linting is the only thing that happened. I ruined some code format and made a commit just for that. After linting, there were no changes and it still committed. I wonder how it does that.

turnerhayes commented 5 years ago

  1. The lint-staged package should be passing staged file names to npm run lint, which should only lint those files. Is it not doing that? Also, it should add them--that's what the "git add" line is supposed to do.

  2. Interesting. I do wonder if that might leave us open to missing bugfixes released under minor/patch versions.

  3. Personally, it doesn't bug me, but I'm also not attached to it so I'm absolutely fine to shush it when possible. It sounds like the PR you linked to would accomplish that, assuming someone fixes the merge conflicts and merges it in.

  4. Yeah, apparently empty git commits are a thing. Seems weird to me too :man_shrugging:

knod commented 5 years ago

  1. It was not doing that. I was in a pre-hook version that needed linting that had no file changes. When I switched to the hook version, made changes in a different file, and committed, that first file was linted too.

  2. True, but then we can explicitly increment the versions. I'd rather get rid of this package-lock issue.

  3. I'm not committed to either path, so maybe someone else can chime in with pros and cons. It at least doesn't have to stop us from merging changes for now if they're otherwise agreed-on.

knod commented 5 years ago

At some point we made a discovery about what was going on [with 'lint-staged']. If anyone can remember it, hit me up so we can put it in here.