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

Breaking: allow style lints during development #82

Closed
wants to merge 1 commit into from

Conversation

niedzielski
Copy link
Contributor

This patch the lowers all style concerns from errors to warnings. The
change allows users to focus on true development concerns-- programmer
errors and functionality instead of prettiness and whitespace
distractions prior to committing. ESLint is excellent at flagging common
JavaScript mishaps that are true bugs but when style concerns are
elevated to the same level as legitimate issues, the latter are much
more difficult to spot.

Since stylish readability is very important as soon as a commit is ready
to be reviewed and any time thereafter, upgrading clients are strongly
encouraged to specify --max-warnings 0 in their pre-commit tests (or
use the supplied tool new to this patch, eslint-wikimedia).

Although this change in itself isn't breaking since it's lowering
severity, most clients are expected to want to change the way they
invoke ESLint to avoid unwanted style offenders creeping in.

This patch the lowers all style concerns from errors to warnings. The
change allows users to focus on true development concerns-- programmer
errors and functionality instead of prettiness and whitespace
distractions prior to committing. ESLint is excellent at flagging common
JavaScript mishaps that are true bugs but when style concerns are
elevated to the same level as legitimate issues, the latter are much
more difficult to spot.

Since stylish readability is very important as soon as a commit is ready
to be reviewed and any time thereafter, upgrading clients are strongly
encouraged to specify `--max-warnings 0` in their pre-commit tests (or
use the supplied tool new to this patch, eslint-wikimedia).

Although this change in itself isn't breaking since it's lowering
severity, most clients are expected to want to change the way they
invoke ESLint to avoid unwanted style offenders creeping in.
@jdforrester
Copy link
Member

I think this is a really bad idea. This is so big it isn't even a breaking change, it's a totally different conceptual item. Maybe you should have a different rule set?

@niedzielski
Copy link
Contributor Author

As to the value for developers, I can't make a much clearer case for this change than the commit message already does. However, perhaps the integration notes could be improved? Although the numbers of lines changed in the diff is great, the only alteration clients will actually need to make to maintain their current lint preferences is to call the new eslint-wikimedia executable instead of eslint directly. This will allow them to continue to extend eslint-config-wikimedia, override rules, add plugins, etc, but now warnings would be considered errors. I agree that initially the change looks alarming or even dangerous but I believe the diff is misleading when the effect is considered.

I personally feel strongly that this is the best way to use ESLint so I'm happy to start the discussion on wikitech-l if you're open to it. I'm also aware that you maintain this library and don't wish to overstep my bounds, so please let me know if you would rather pass on this change and I can consider the matter concluded.

Thanks you for reviewing!

@jdforrester
Copy link
Member

I personally feel strongly that this is the best way to use ESLint

OK. I feel strongly that it is not.

I'm happy to start the discussion on wikitech-l if you're open to it

Sure.

@niedzielski
Copy link
Contributor Author

Thanks @jdforrester, will do. It occurred to me some time after writing the previous comment that perhaps a testament to the lack of integration changes needed are the very tests in eslint-config-wikimedia itself! 0 changes!! Stay tuned and we can continue the discussion on wikitech-l! 👍

@edg2s edg2s closed this Dec 1, 2018
@jdforrester jdforrester deleted the breaking/warn branch October 2, 2019 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants