Skip to content

WIP: Use husky to execute linters - #27982

Closed
MartijnCuppens wants to merge 1 commit into
v4-devfrom
v4-dev-martijncuppens-husky
Closed

MartijnCuppens wants to merge 1 commit into
v4-devfrom
v4-dev-martijncuppens-husky

Conversation

@MartijnCuppens

@MartijnCuppens MartijnCuppens commented Jan 5, 2019 •

Copy link
Copy Markdown
Member

Closes #27973

The linting taks are now ran before anything is pushed which will help us with builds that fail because of trivial lint errors.

I preferred the pre-push option above the pre-commit option because all linting tasks take about 9 seconds to complete which could be annoying.

This PR also contains a commit which parallelizes the linting tasks. This decreases the time from around 9 seconds to 4-5 seconds.

Not sure if we should add the --fix command which automatically fixes fixable linting errors (like fixing the property order). Maybe we could add an additional command for it?

TODO:

  • Add documentation once we've decided how we're going to implement it.

@MartijnCuppens MartijnCuppens added the build Build scripts, bundling, dist/ label Jan 5, 2019
@XhmikosR

XhmikosR commented Jan 5, 2019

Copy link
Copy Markdown
Member

I still find it too slow.

@XhmikosR

XhmikosR commented Jan 5, 2019

Copy link
Copy Markdown
Member

You can split the parallel patch to a separate PR so that we try it and merge it regardless of this.

@MartijnCuppens MartijnCuppens added the on-hold Paused by a maintainer label Jan 5, 2019
@Johann-S

Johann-S commented Jan 7, 2019

Copy link
Copy Markdown
Member

It's a bit slow 😟 do you know if there is a way to skip that on push ?

@MartijnCuppens

Copy link
Copy Markdown
Member Author

#27989 will increase speed to around 1.8s (for me) which is acceptable for me as pre-push, don't know what you guys think? Imo it's worth it because figuring out what's wrong with some one else's PR takes a lot more time.

--no-verify can be used to skip the tests.

@XhmikosR

XhmikosR commented Jan 7, 2019

Copy link
Copy Markdown
Member

Or we could just leave things as is, and you can add the hook manually.

@MartijnCuppens

Copy link
Copy Markdown
Member Author

It's not only for me I opt for this. A lot of Travis tests are failing because of linting issues and these hooks can spare contributors some time because it will give linting feedback before anything is pushed. It would also save us some time figuring out what's wrong with the Travis tests and replying they should check their linting or fixing the code ourselfs.

On the other hand it slows down our current workflow and is yet another dependency where thing could go wrong with.

@XhmikosR

XhmikosR commented Jan 8, 2019

Copy link
Copy Markdown
Member

The thing is, many people don't even check out the repo locally and even if they do they don't run the test scripts.

Secondly, we do have CI for this anyway.

I'm not against it as long as it doesn't disrupt my current workflow, and as it seems it does since it slows us down. The dep I don't mind, it's a small one anyway.

Also, IMO this should be in precommit if we ever go with it. You could try calling the individual css and js lint scripts if that's faster instead of the wrapper lint script.

@MartijnCuppens
MartijnCuppens requested review from a team as code owners January 12, 2019 17:50
@XhmikosR

Copy link
Copy Markdown
Member

That's not a rebase.

@MartijnCuppens
MartijnCuppens force-pushed the v4-dev-martijncuppens-husky branch from f2738da to 98c6ce2 Compare January 12, 2019 19:53
@MartijnCuppens

Copy link
Copy Markdown
Member Author

Fixed it

@XhmikosR

Copy link
Copy Markdown
Member

You missed the update package-lock.json.

@MartijnCuppens
MartijnCuppens force-pushed the v4-dev-martijncuppens-husky branch from 98c6ce2 to 1094e90 Compare January 13, 2019 14:19
@XhmikosR
XhmikosR force-pushed the v4-dev-martijncuppens-husky branch from 1094e90 to 0fa9335 Compare February 12, 2019 15:17
@XhmikosR

Copy link
Copy Markdown
Member

OK, I tried this again, and it's just too slow for me...

@MartijnCuppens

Copy link
Copy Markdown
Member Author

Ok fine, I'll close it than

@MartijnCuppens
MartijnCuppens deleted the v4-dev-martijncuppens-husky branch February 12, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Build scripts, bundling, dist/ on-hold Paused by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants