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

(chore) use eslint on src files #2602

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 11, 2020

This is an attempt to answer @yyyc514's request on #2601 (comment).

This PR is adding eslint to the test command, to enforce rules on source files. I have also run eslint --fix on the source files.

I have changed a few rules from the eslintrc file:

  • Remove TypeScript parser and plugins. This repo doesn't use TS at all, it was probably an oversight.
  • Disable no-useless-escape on the ./src/languages/… files. There are just too many of them.

@joshgoebel
Copy link
Member

joshgoebel commented Jun 11, 2020

Sorry but I don't think we can merge this one.

I have also run eslint --fix on the source files.

This destroys/pollutes Git history. I'm usually against large style changes like this because as a maintainer I find clean history to be MUCH more valuable than perfect style. (and the style issues here aren't terrible) We'll improve them with time.

This PR is adding eslint to the test command, to enforce rules on source files.

Maybe one day, not today. I don't think adding more friction for maintainers/contributors right now is a win.

Out of curiosity: Is there perhaps some easy way to only lint MODIFIED files?

Remove TypeScript parser and plugins. This repo doesn't use TS at all, it was probably an oversight.

It's used in "jsonly" mode and for all the type inference it can do plus the JSDoc types really helps with catching errors while coding. It supplements the linter nicely in this regard.

Disable no-useless-escape on the ./src/languages/… files. There are just too many of them.

I have definitely consider doing this. :-) I'm still debating if they add some type of documentation value (better a useless escape than missing a REAL escape) or if they are truly just clutter and should all be cleaned up.

@joshgoebel joshgoebel closed this Jun 11, 2020
@joshgoebel
Copy link
Member

I might be more open to adding this (lint on test) for JUST the core parser source (excluding languages for now)... I might experiment with that locally and see how that works out.

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 11, 2020

better a useless escape than missing a REAL escape

Actually there are some that outline real issues:

begin: '^---\s*$',

Here eslint repports Unnecessary escape character: \s, and indeed the backslash should be escaped instead of the s:

  begin: '^---\\s*$',

Is there perhaps some easy way to only lint MODIFIED files?

I think solutions exist involving commit hooks. I've never use it myself though.

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 11, 2020

I might be more open to adding this (lint on test) for JUST the core parser source (excluding languages for now)... I might experiment with that locally and see how that works out.

FWIW I think that would be the right approach. Maybe let the linter check the languages modules but disable all the rules that do not currently pass?

@joshgoebel
Copy link
Member

Maybe let the linter check the languages modules but disable all the rules that do not currently pass?

That's kind of what I do now, but I don't feel any compulsion to "update them all at once". I keep an eye on the linter warnings (they are toned way done for languages) when I'm updating a language... but if I'm making larger changes I might quickly comment out all the language rules to get a much stricter ruleset in place and update some styles while I'm in there.

@joshgoebel
Copy link
Member

Added task for now: dfeb3a1

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.

2 participants