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

Add eslint rules and fix errors #3726

Merged
merged 1 commit into from Mar 8, 2018
Merged

Conversation

@bansalnitish
Copy link
Contributor

@bansalnitish 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
@davidfischer davidfischer changed the title Add lint rules and fix errors Add eslint rules and fix errors Mar 5, 2018
@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Mar 5, 2018

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

@davidfischer
Copy link
Contributor

@davidfischer 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
Copy link
Contributor Author

@bansalnitish 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
Copy link
Contributor Author

@bansalnitish bansalnitish commented Mar 7, 2018

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

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Mar 7, 2018

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

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 8, 2018

I'll let @davidfischer do the honors!

@davidfischer
Copy link
Contributor

@davidfischer 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
Copy link
Contributor Author

@bansalnitish bansalnitish commented Mar 8, 2018

Sure @davidfischer, I will make a separate PR!

@davidfischer
Copy link
Contributor

@davidfischer 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 readthedocs:master Mar 8, 2018
1 check passed
@bansalnitish bansalnitish deleted the lint-fix branch Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants