ricardofbarros / linter-js-standard

Atom linter plugin for JavaScript, using JavaScript Standard Style
https://atom.io/packages/linter-js-standard
MIT License
99 stars 48 forks source link

old warnings are now errors #199

Closed oscarojeda closed 7 years ago

oscarojeda commented 7 years ago

old warnings are now errors

xclidongbo commented 7 years ago

me too.

thang2162 commented 7 years ago

Apparently this was changed in the latest version to conform better with the standard style. However, the change seems to have been made across all styles not just standard which seems like a bug to me. For now, I've rolled back to the previous version using: apm install linter-js-standard@3.9.3 from the command line.

sonicdoe commented 7 years ago

As @thang2162 already mentioned, this change is intentional. linter-js-standard now respects ESLint’s severity property which — in the case of standard (and probably all other standard-based styles) — means that all previous warnings are now displayed as errors since they only use the error severity (see, for example, standard’s eslintrc.json).

Anyway, I’m interested in hearing arguments for treating all ESLint errors as warnings. For what it’s worth, this change makes linter-js-standard more consistent with standard itself and matches behavior of linter-eslint and atom-linter-xo, for example.

thang2162 commented 7 years ago

I assumed as much altough it maybe better to switch it back as it would restrict the usefulness of the linter in my view especially when trying to analyze code snippets. In mine and I would guess many other cases, we've written a lot non conforming but working code which is now all getting flagged as error rendering the linter completely pointless.

On Wed, Aug 30, 2017 at 5:33 AM Jakob Krigovsky notifications@github.com wrote:

As @thang2162 https://github.com/thang2162 already mentioned, this change is intentional. linter-js-standard now respects ESLint’s severity property which — in the case of standard (and probably all other standard-based styles) — means that all previous warnings are now displayed as errors since they only use the error severity (see, for example, standard’s eslintrc.json https://github.com/standard/eslint-config-standard/blob/master/eslintrc.json ).

Anyway, I’m interested in hearing arguments for treating all ESLint errors as warnings. For what it’s worth, this makes linter-js-standard more consistent with standard itself and matches behavior of linter-eslint https://github.com/AtomLinter/linter-eslint and atom-linter-xo https://github.com/sindresorhus/atom-linter-xo, for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ricardofbarros/linter-js-standard/issues/199#issuecomment-325951492, or mute the thread https://github.com/notifications/unsubscribe-auth/ADkzx2zOibpkY8L6gvhqgiSu6eqtVj4cks5sdTprgaJpZM4PE_TW .

Arcanemagus commented 7 years ago

@thang2162 This isn't changing what gets flagged, only how it is classified. I'm rather curious how that makes this "completely pointless"?

Eldar-X commented 7 years ago

I think it would be more appropriate to separate them according to their importance.

For example, forgetting to define a variable is a error; but leaving extra space in the code is a warning that does not really harm your application.

Especially like me, those who use this plug-in for the first time think they make a logical mistake, and the last thing that comes to mind is that we forget to leave some space. I think it causes distraction

sonicdoe commented 7 years ago

I can see where you are coming from, however, standard decided on errors only (see https://github.com/standard/standard/issues/108#issuecomment-91108450) and I think it’s not really in the realm of linter-js-standard to classify standard’s ESLint rules.

Having said that, there may be value in treating certain ESLint rules (such as stylistic issues) as warnings. I’d be inclined to include this as an option if there would be a reliable categorization of standard’s ESLint rules.

Arcanemagus commented 7 years ago

For those interested in changing the warning level you might want to move to a full ESLint configuration based on eslint-config-standard (essentially this is all that standard is...). This way you can override the rule's configuration (such as the reporting level) to whatever you wish.

This does go against the entire point of standard, but since it seems you don't agree with the choices that standard has made that is the appropriate path forward.

gigitux commented 7 years ago

Would be great if there will be an option for restore old behavior.

refactorized commented 7 years ago

So im dropping this package now, and probably moving to es-lint based closely on standard.

You wanna sell one kind of bike shed to make life simpler thats fine. but tell me I can't cut a hole where I need one and I'm just going to have to burn it down.

sonicdoe commented 7 years ago

Since this is unlikely to change soon, I’m going to close this issue now.

To recap, I’d be open to treating certain ESLint rules as warnings (see https://github.com/ricardofbarros/linter-js-standard/issues/199#issuecomment-328335103) but I don’t see the benefit of converting all standard errors to warnings (aside from it working like that before).

If you’d like to configure which ESLint rules are treated as warnings, I’d recommend you go with @Arcanemagus’s suggestion in https://github.com/ricardofbarros/linter-js-standard/issues/199#issuecomment-328365515:

For those interested in changing the warning level you might want to move to a full ESLint configuration based on eslint-config-standard (essentially this is all that standard is...). This way you can override the rule's configuration (such as the reporting level) to whatever you wish.