Skip to content

Conversation

@daiyam
Copy link
Contributor

@daiyam daiyam commented Oct 1, 2018

This change is running the lint command before a commit.

@jacobherrington
Copy link
Contributor

I personally think forcing lint on commits is a bit heavy handed - our CI already runs lint so I don't think it's totally necessary.

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Oct 1, 2018

I think with pre-commit hook we can detect the problem right away without waiting for the CI to fail and prevent some unnecessary commit like "fixed lint" 😄

@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Oct 5, 2018
@nagledb
Copy link
Contributor

nagledb commented Oct 25, 2018

Maybe also have it run the tests?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 6, 2018

LGTM but can you discard yarn.lock? Also, I think running test in the pre-commit hook is a bit heavy for small changes (There's still some chance that small changes can cause failed tests, but I think leaving it to the CI is the best). @daiyam

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 6, 2018
@daiyam
Copy link
Contributor Author

daiyam commented Dec 6, 2018

@ZeroX-DG I agree with you since the tests are heavy.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Dec 6, 2018
@Rokt33r Rokt33r added next release (v0.11.13) and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Dec 24, 2018
@Rokt33r
Copy link
Member

Rokt33r commented Dec 24, 2018

@ZeroX-DG @daiyam I think tests should be executed before pushing. So how do you think add pre-push hook for testing?

@Rokt33r Rokt33r merged commit b018502 into BoostIO:master Dec 24, 2018
@daiyam daiyam deleted the precommit-command branch December 24, 2018 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants