Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project wide editorconfig lint #224

Merged
merged 3 commits into from
Dec 14, 2018

Conversation

aslafy-z
Copy link
Collaborator

  • I've checked that this isn't a new swag opportunity proposal.
  • I've checked that this isn't a duplicate pull request.

Use https://github.com/jedmao/eclint to check and fix lint respecting rules from .editorconfig file.

Added to the lint-staged pipeline.

LICENSE is set to be ignored as it does not respect any rules, we can also add a specific rule for it instead of ignoring it.

@aslafy-z aslafy-z added the enhancement New feature or request label Dec 11, 2018
@swapagarwal
Copy link
Owner

Why moving away from XO?

@aslafy-z
Copy link
Collaborator Author

We are not! xo is a js only linter.
We use eclint to lint all other files (pug, styl, html, json, ...)

@swapagarwal
Copy link
Owner

Oops. My bad! 😅

.editorconfig Outdated Show resolved Hide resolved
src/styl/imports/base.styl Outdated Show resolved Hide resolved
Copy link
Contributor

@vikaspotluri123 vikaspotluri123 left a comment

Choose a reason for hiding this comment

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

I think this is good to merge, I haven't run any tests locally since it should all be visual. My comment is for general discussion, not blocking 😊

@@ -63,7 +64,12 @@
},
"lint-staged": {
Copy link
Contributor

Choose a reason for hiding this comment

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

General question: is this absolutely required? I know there are lots of repos that do this, but (for me personally) it wastes a lot of time because I have to wait for lint to run until I can commit + push to remotes and it makes me not want to commit because there's too much friction

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. I would prefer a pre-push hook rather than pre-commit

Copy link
Contributor

Choose a reason for hiding this comment

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

With CI, failing lint prevents merge

Copy link
Collaborator

Choose a reason for hiding this comment

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

But CI is after pushing changes, right? And CI evaluates the last commit.

Copy link
Collaborator Author

@aslafy-z aslafy-z Dec 13, 2018

Choose a reason for hiding this comment

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

Something like this: https://github.com/theenadayalank/lint-prepush?
I'll have to drop the --fix option if we go that way as it does not support creating a lint-fix commit. Push will fail if lint is not OK.

The lint-staged task runs really fast on my computer, is it slow for you? I think keeping the lint-fixes within the commit is better but if you guys want to ditch it, we can.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't we just do this:

"pre-push": "npm run lint:fix"

Copy link
Collaborator Author

@aslafy-z aslafy-z Dec 13, 2018

Choose a reason for hiding this comment

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

If the task returns without errors (even if it linted some files, unstaged so), it will continue to push without creating a fix commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, okay, I understand.

I'll have to drop the --fix option if we go that way as it does not support creating a lint-fix commit. Push will fail if lint is not OK.

I'm fine with this. But if we decide to continue with the pre-commit hook, I'm fine with that too 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

The lint-staged task runs really fast on my computer, is it slow for you?

Yep, adds 3-5 seconds, even though I have an ssd 😭

Copy link
Contributor

@itaisteinherz itaisteinherz Dec 13, 2018

Choose a reason for hiding this comment

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

I usually see projects checking for linting errors only on CI. If you decide on going in this route, I think it's preferable to have linting done automatically on push instead of on every commit, since the latter makes the entire iteration process slower and more annoying.
Regarding creating a lint-fix commit automatically on push / commit, that doesn't seem like a good idea to me, since many times I've had auto-fixable linting errors which I needed to fix manually as that was more appropriate.

These are just my opinions and observations from working on other projects though.

@aslafy-z
Copy link
Collaborator Author

Can I merge this?

@vikaspotluri123
Copy link
Contributor

👍

@aslafy-z aslafy-z merged commit 9daceb7 into swapagarwal:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants