Skip to content

Changed ESLint rule configs to use 'off', 'warn', and 'error' instead of numbers for better readability #946

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

Merged
merged 1 commit into from
Aug 20, 2016

Conversation

yangsu
Copy link
Contributor

@yangsu yangsu commented Jul 9, 2016

I wrote a JsCodeShift script to accomplish this.

@ljharb
Copy link
Collaborator

ljharb commented Jul 9, 2016

Thanks for the contribution! However, I prefer sticking with the historical 0/1/2 notation of jslint/jshint/eslint.

@yangsu
Copy link
Contributor Author

yangsu commented Jul 9, 2016

The main reason for me submitting this pull request is that the latest documentation for eslint rules all use the string enums rather than numbers.

@ljharb
Copy link
Collaborator

ljharb commented Jul 9, 2016

Understood, and that's a fair point.

I'll leave this open for the time being so other contributors can weigh in (heads up, it'll likely need to be rebased and reran before merging, if it comes to that)

@kesne
Copy link
Contributor

kesne commented Jul 9, 2016

I'm 👍🏻 to this change. It seems to me as though it makes the configuration more readable, which is what we should always strive for.

@lencioni
Copy link
Contributor

lencioni commented Jul 9, 2016

I also see this as a positive change, for the same reasons already stated.

@yangsu
Copy link
Contributor Author

yangsu commented Jul 13, 2016

I rebased and reran the codeshift script.

@hshoff
Copy link
Member

hshoff commented Jul 25, 2016

Looks reasonable to me

@yangsu
Copy link
Contributor Author

yangsu commented Jul 26, 2016

Any other suggestions/thoughts before this can be merged? Otherwise I'll have to keep rebasing and re running the codeshift

@ljharb
Copy link
Collaborator

ljharb commented Jul 26, 2016

At the very least, this needs to wait until after #936 is resolved, since I have those commits queued up and don't want the conflict.

I still do not think this is a good change, fwiw, but enough contributors have weighed in that I'm ok accepting it once the new release is out.

@intentionally-left-nil
Copy link

+1 to this change: as more time goes forward, more code is just going to be using "off", "warn", and "error". Newer folks (like myself), then have to go back and look back to see what the old numbers mean.

@ljharb
Copy link
Collaborator

ljharb commented Aug 18, 2016

@yangsu if you'd like to rebase this one last time, i'll merge it.

@yangsu
Copy link
Contributor Author

yangsu commented Aug 20, 2016

Done

@ljharb ljharb merged commit 2e69472 into airbnb:master Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants