wawandco / milo

Milo is an HTML linter written in Go
MIT License
20 stars 0 forks source link

Warning message about missing .milo.yml is misleading users into possibly doing the wrong thing. #46

Closed christianhujer closed 4 years ago

christianhujer commented 4 years ago

When running Milo without configuration (no run of milo init, thus no file .milo.yml present), Milo prints the following message:

[Warning] could not load configuration from .milo.yml

Warnings are something users should fix. The fix is to run milo init. And that is not a good idea.

When running milo init to create a .milo.yml file, this file has an include-list of all checks present in milo at the time of running milo init. When updating Milo, users expect to get and run new checks. But this won't happen unless they delete and re-create or modify their .milo.yml files, which most likely they will forget.

If .milo.yml is not present, Milo should not print any message. Instead, the help text should tell users about .milo.yml.

Also, the list of checks in .milo.yml should rather be an exclude-list to disable checks, not an include-list to enable checks, to protect users from missing out on new checks when updating Milo.

paganotoni commented 4 years ago

Hey @christianhujer, I think there are two topics on this one. I agree with the first one, we may need to present the absence of the .milo.yml file in a better way for the users and use that space to explain a bit more.

On the second topic I think we would want to provide some sort of consistency for our users. While I see the case for wanting to run all the linters I think by having these specified will make clear and visible the reviewers that are running. Maybe we can document that if you leave the list of reviewers empty Milo will run all of the reviewers.

paganotoni commented 4 years ago

I'm also not sure of how complex the .milo.yml file will get with time. I would say we want to keep it simple. That said I know there are a lot of use cases we are not covering still, hopefully by v1.0.0 we can cover most of them.

christianhujer commented 4 years ago

Whenever Nu Validator, SonarLint, CheckStyle, PMD, KtLint, FindBugs, Jigsaw CSS Validator, and so on get new checks, they don't have to be enabled explicitly. I suggest keeping Milo consistent with the majority of the linters out there.

If at all, checks could be categorized into two groups:

After all, the purpose of running a tool like Milo is to find issues first, and then decide that some of the issues are non-issues. If in doubt, Milo should always lean towards reporting instead of non-reporting, and a configuration file with an include-list of checks goes against that philosophy.

The only linter that I know in which new rules occasionally have to be enabled explicitly is PC-Lint (for C/C++). And PC-Lint is an entirely different (awesome) beast.

Also from the perspective of keeping things simple, an exclude-list might be a much shorter .milo.yml file in the end than an include-list.

paganotoni commented 4 years ago

Changed the warning message to be a bit more informational:

Running all reviewers, see more details in: https://github.com/wawandco/milo#configuration.