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

Add eslint rules and fix errors #3726

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
5 participants
@bansalnitish
Contributor

bansalnitish commented Mar 3, 2018

This PR Fixes #2909.

I have tested for all the rules mentioned in the issue. I have set the rules to error level when there are no false positives otherwise it is set to warning level.

There were 106 errors and warnings. Now we have 0 errors and 31 warnings. All these warnings are false positives.

I would also like to suggest to enforce the following rules:

  • indent (especially this)
  • semi
  • space-unary-ops
  • space-before-function-paren
  • no-unused-vars

@ericholscher ericholscher requested review from davidfischer and agjohnson Mar 5, 2018

@davidfischer davidfischer changed the title from Add lint rules and fix errors to Add eslint rules and fix errors Mar 5, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Mar 5, 2018

I changed the title to note that this is ESLint as opposed to PyLint.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Mar 5, 2018

Firstly, thank you for the first time contribution.

This looks well thought through and well done and I'm happy to see this make it into master and get run on every commit! I was able to run tox -e eslint without issue and I see the same warnings and no errors. I think the changes to the .eslintrc are reasonable.

I do think your suggestions (especially indent) are good ideas. However, there's probably a lot of fixes around those. I don't think those other fixes should go in this PR.

@bansalnitish

This comment has been minimized.

Contributor

bansalnitish commented Mar 6, 2018

Firstly, thank you for the first time contribution.
This looks well thought through and well done and I'm happy to see this make it into master and get run on every commit!

Thank you @davidfischer!

However, there's probably a lot of fixes around those. I don't think those other fixes should go in this PR.

Shall I make a separate PR for that?

@bansalnitish

This comment has been minimized.

Contributor

bansalnitish commented Mar 7, 2018

Hi @davidfischer, is there anything else I need to do here?

@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Mar 7, 2018

I believe this is just waiting for @ericholscher to merge. :)

@ericholscher

This comment has been minimized.

Member

ericholscher commented Mar 8, 2018

I'll let @davidfischer do the honors!

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Mar 8, 2018

I'll plan to merge this in the next hour or so today.

@bansalnitish, sure a separate PR would be great.

@bansalnitish

This comment has been minimized.

Contributor

bansalnitish commented Mar 8, 2018

Sure @davidfischer, I will make a separate PR!

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Mar 8, 2018

For indentation, I ideally let's stick to 4 spaces similar to Python. https://eslint.org/docs/rules/indent

@davidfischer davidfischer merged commit e923c0c into rtfd:master Mar 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bansalnitish bansalnitish deleted the bansalnitish:lint-fix branch Mar 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment