Puppet linter doesn't respect severity maps, although listed in config options. As puppet-lint returns code and severity in separate fields, ArcanistLinter::getLintMessageSeverity() should take an optional second parameter.
Details
Details
- Reviewers
joshuaspence epriestley - Group Reviewers
Blessed Reviewers - Required Signatures
L28 Phacility Individual Contributor License Agreement
Tested in a puppet repo with .arclint set up like this:
"severity.rules": { "(.*)": "advice" }Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Branch
- puppet-lint-respect-severity
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 4827 Build 4843: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
| src/lint/linter/ArcanistPuppetLintLinter.php | ||
|---|---|---|
| 119–141 | I think that it would be simpler to use getLintMessageSeverity($matches[2]) here and provide an implementation of the getDefaultMessageSeverity method. | |
| src/lint/linter/ArcanistPuppetLintLinter.php | ||
|---|---|---|
| 119–141 | Sorry, I don't understand your suggestion; doesn't my patch work towards this direction? It's been a while, apologies if I'm missing something. | |
Comment Actions
See some discussion in D11657.
- The linter should pass the most specific code it can to setCode().
- Ideally, each message would have a unique code.
- If each message has a unique code, users can tune the severity of each message.
- If you can't provide a unique code, provide the most specific code you can. For example, this linter might only be able to define very broad codes like "PUPPETLINT-ERROR" and "PUPPETLINT-WARNING".
- The value passed to setCode() should completely determine the message severity on its own. If the value is not specific enough (as here), you should increase the specificity of the code.
- You should not make changes to ArcanistLinter.php.
- Until D11657, you should set severity with ->setSeverity($this->getLintMessageSeverity($message->getCode())) exactly. Some time after D11657, we will likely make this implicit so it becomes impossible to get wrong.
Specifically, the flow of execution will go like this:
- getLintMessageSeverity() accepts the code.
- It checks severity and severity.rules to see if the user has customized the severity for the code.
- If not, it calls getDefaultMessageSeverity($code).
- That returns 'error' for "PUPPET-ERROR", 'warning' for "PUPPET-WARNING", etc. Ideally, the codes would be more specific than this to allow users greater control through severity.
Comment Actions
My understanding is that you want to merge severity and code into one field. IMHO this does not seem to be a good fit for puppet-lint, which, perhaps unlike linters like pep8, provides the two in separate fields and does not output codes that look like stable short IDs, as far as I can tell. I understand that all linters should conform to the same standard, but I think this will result in ugly codes that may be somewhat unpredictable for users to match.
Here goes anyway though. I am always embedding puppet-lint provided severity in the code, as I need to pass that to getDefaultMessageSeverity() and there is no other way to do that, except for extending ArcanistLintMessage with e.g. a $severity_external_linter property, which I don't want to do (I'm not sure if you'd want me to either).
Comment Actions
I understand your concerns but I am a bit frustrated it may take so long to tackle something that should have been trivial to fix. I will probably abandon this unless there is quick progress in T8474 or you are willing to consider an interim solution.
Comment Actions
See this document for discussion on why this is taking a long time:
https://secure.phabricator.com/book/phabcontrib/article/contributing_code/