garage-it / SmartHouse-backend

9 stars 2 forks source link

Remove git hooks #59

Closed borys-ihnatyev closed 7 years ago

borys-ihnatyev commented 7 years ago

How about remove lint and test run from pre-commit hook?

Could you please share your opinion about this?

sergii-trotsiuk commented 7 years ago

In this case not checked commits will be merged to master branch after pull request approve.

kolesnik commented 7 years ago

I am agree with @borys-ihnatyev it is better to use IDE lint validation, and we are not merging pull requests with red builds (if validation is failing)

disist commented 7 years ago

I am agree also, this improvement can provide comfortable developing process for us, without transitional stuff.

stremann commented 7 years ago

@borys-ihnatyev could you create a pull request for that?

sergii-trotsiuk commented 7 years ago

@kolesnik I talk not about red builds I talk about commits which contain not checked code. For example, developer made commit C1 without lint validation and after that made commit C2 with fixes according to lint validation. CI server allow pull request and C1 commit merge to master.

kolesnik commented 7 years ago

yep it's possible but this PR will not pass review

sergii-trotsiuk commented 7 years ago

So reviewer should check each commit for as lint on code review? Not diff tab and just logic mistakes? I think it doesn't work. So if we move lint validation from Git hooks to CI server, we must accept the fact that some commits in master will not be validated with lint. @borys-ihnatyev Why do you want remove lint from Git hooks?

kolesnik commented 7 years ago

It is not possible when we are merging only PRs with green builds. Also we are doing "Squash and merge" for each PR it means that only one validated commit is placed to the master.

sergii-trotsiuk commented 7 years ago

Squash fix this situation. Just in case, @kolesnik are you going to save npm script for lint validation?

kolesnik commented 7 years ago

I don't think that we need separate script for that, we are sharing lint config via .eslintrc.json

sergii-trotsiuk commented 7 years ago

How do you propose run lint on CI server? During production build script (npm script)?

borys-ihnatyev commented 7 years ago

@sergey-trotsyuk We have CI to lint our code (pull requests), and if you want you can lint manually on your machine before pull request (npm run lint) or turn on eslint in WebStorm or your IDE/Text editor. I am not sure why should we care about every commit to be linted, it is out of process to push directly into master.

borys-ihnatyev commented 7 years ago

@sergey-trotsyuk we already have lint check on CI https://github.com/garage-it/SmartHouse-backend/blob/master/.travis.yml#L12

sergii-trotsiuk commented 7 years ago

@borys-ihnatyev So we save lint script. Ok, let's try.

borys-ihnatyev commented 7 years ago

59