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

Set severity level of rules in preset, as remark(-lint) has no option to pass the level through a preset plugin #36

Open
dietergeerts opened this issue Aug 13, 2020 · 10 comments

Comments

@dietergeerts
Copy link
Contributor

By default, if no severity is set, it seems that remark-lint sees the messages as warnings. Imho, it makes more sense that rules that must be obeyed to correctly follow the spec are listed as "error", no? If one would like to set them as warning, then they can overwrite it. Off course, this change would be a breaking change, so @RichardLitt , how would you like this improvement be handled in relation to versioning?

dietergeerts added a commit to dietergeerts/standard-readme-preset that referenced this issue Aug 13, 2020
@RichardLitt
Copy link
Owner

Do errors stop builds? If warnings don't, it may actually foster adoption of the standard by not enforcing it when people want to slightly modify their READMEs to match their particular use-case.

@wooorm
Copy link
Collaborator

wooorm commented Aug 14, 2020

remark is not just a linter, it can also process and format. Hence, if formatting succeeds, we don’t exit with an error, even if there are warnings.

As this is a preset, and the remark CLI has a --frail flag so that it does exit as an error if there were warnings, I’d say to keep in line with remark, as it currently is, and not merge this!

@RichardLitt
Copy link
Owner

Thanks for clarifying, @wooorm! Leaving as is.

@dietergeerts
Copy link
Contributor Author

dietergeerts commented Aug 16, 2020

remark is not just a linter, it can also process and format. Hence, if formatting succeeds, we don’t exit with an error, even if there are warnings.

As this is a preset, and the remark CLI has a --frail flag so that it does exit as an error if there were warnings, I’d say to keep in line with remark, as it currently is, and not merge this!

I understand that --frail can be used, but that would let the run fail on ANY warning. The linting rules provided by this preset are used to report actual "errors" according to the specs. If this is used with other plugins/presets, then --frail can't be used if only the actual "errors" are to be resulting in the process ending in an error. So @wooorm , what do you propose to be able to do this? As with --frail, this can't be done.

@dietergeerts
Copy link
Contributor Author

dietergeerts commented Aug 16, 2020

I looked into several presets, and there is like no rule in what should be used. Some don't set a level, other do like https://github.com/raduserbanescu/remark-preset-lint-styleguide/blob/master/src/index.js. So it would be interesting to know what the standard approach should be? The ability to set the severity level was specifically added because --frail fails on all warnings, while in a lot of situations, you want some things to be warnings and not fail the build, while others should be errors and fail the build. (remarkjs/remark-lint#65) Though it was implemented, the docs don't subscribe very well how it is intended to be used in combination with presets and what the default usage should be. So I would like to know more, so that we can find out what we can do in this preset in order to make it easier to use.

@RichardLitt RichardLitt reopened this Aug 17, 2020
@RichardLitt
Copy link
Owner

@wooorm is the expert on this one.

@wooorm
Copy link
Collaborator

wooorm commented Aug 18, 2020

So @wooorm , what do you propose to be able to do this? As with --frail, this can't be done.

I suggest that errors which crash, be errors (e.g., misconfiguration or so), and that errors that are stylistic (e.g., linting, this project), be warnings. And that --frail mode is used if you specifically want to crash on stylistic issues too. I do not suggest setting different values in presets shared on npm, but it could be useful if you are so inclined to make a preset for your org/company/project, which differs from the defaults.

The preset you linked to sets an error everywhere: that could’ve just as well been --frail. The OP of the original issue you linked to is currently, 4 years later, doing error only too. I believe both those cases could’ve then used --frail.

looked into several presets, and there is like no rule in what should be used.

There is no hard rule indeed, but my strong suggestion, and what most folks are doing, is to use the defaults: warnings.

I agree with what Richard said at the start:

Do errors stop builds? If warnings don't, it may actually foster adoption of the standard by not enforcing it when people want to slightly modify their READMEs to match their particular use-case.

Finally, presets are a combination of plugins. This one is 7 lines. If you want to differentiate here, or combine with other things, you can make your own! Here’s mine!

@dietergeerts
Copy link
Contributor Author

dietergeerts commented Aug 18, 2020

and that errors that are stylistic (e.g., linting, this project), be warnings

Though the thing is that linting is not only stylistic, it's also about avoiding errors, confusion etc.... So linting itself, you have rules that point out potential errors, or "errors" in the sense that it will confuse devs and in turn write errors. Other rules are purely stylistic, which are there to just bring consistency, which Imho are not really needed, but some want them.

So the use-case is that when you have both "stylistic" linting rules AND "error-avoiding" linting rules, then you want the linting process to fail on these last kind, so you can take action and fix them.

I agree with what Richard said at the start:

Do errors stop builds? If warnings don't, it may actually foster adoption of the standard by not enforcing it when people want to slightly modify their READMEs to match their particular use-case.

I understand this, though I did observe multiple times that warnings get discarded, and are never solved, which means that in this case the standard will not be adopted, even though it was intended to. However, my intent is not to force my opinions on this preset and change it towards what I want. I just want to help improve for use-cases I have and see are needed. Programming is about trade-offs, and whatever get decided eventually, I'm fine with it.

Finally, presets are a combination of plugins. This one is 7 lines. If you want to differentiate here, or combine with other things, you can make your own! Here’s mine!

Though the point of presets is that you can just add them to your dependencies, and add them to your config, and it just works. Imho, presets must work together to enable to just use them, and use all the ones you need for your project. Less presets out there in the wild makes the OS projects that uses them more consistent, which makes it easier for the devs working on them, and other that are reading through code of these projects.


So I see that there is another view on linting and errors vs warnings. I would like to discuss more about this to find out why you have these views, as I probably will be missing views/parts of why. Apart from the discussion I'm fine with whatever get decided. I already have an idea for a helper that can set the severity level for any preset to the one needed/wanted, so that it can be applied based on the fact that it's "stylistic" vs "error-avoiding".

@RichardLitt
Copy link
Owner

I don't have a lot of views on this. I don't use this module, at the moment, and I don't really use any similar modules that have errors or warnings. My main concern is that managing expectations for people who use this sort of tool, and for that I tend to defer to @wooorm.

@wooorm
Copy link
Collaborator

wooorm commented Aug 23, 2020

So I see that there is another view on linting and errors vs warnings. I would like to discuss more about this to find out why you have these views, as I probably will be missing views/parts of why.

unified does two things: a) check and b) build your site/docs/whatever. An error means that it all stops: it failed. The site can’t or shouldn’t be deployed. In my opinion it makes the most sense that builds succeed if they can, even if there are stylistic problems or inconsistencies. You’re of course free to choose and configure how you want, but that’s why all the checks for unified that I maintain are warnings. And all the incorrectly configured things and such, are errors.

Apart from the discussion I'm fine with whatever get decided. I already have an idea for a helper that can set the severity level for any preset to the one needed/wanted, so that it can be applied based on the fact that it's "stylistic" vs "error-avoiding".

Nice! Sounds useful!

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

No branches or pull requests

3 participants