Page MenuHomePhabricator

Let ArcanistPuppetLintLinter respect severity/severity.rules maps
AbandonedPublic

Authored by zmousm on Mar 10 2015, 5:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 18 2024, 4:14 AM
Unknown Object (File)
Feb 3 2024, 2:24 AM
Unknown Object (File)
Jan 12 2024, 5:25 AM
Unknown Object (File)
Jan 12 2024, 5:25 AM
Unknown Object (File)
Jan 12 2024, 5:25 AM
Unknown Object (File)
Jan 10 2024, 11:11 AM
Unknown Object (File)
Jan 7 2024, 4:49 PM
Unknown Object (File)
Jan 4 2024, 10:44 AM

Details

Summary

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.

Test Plan

Tested in a puppet repo with .arclint set up like this:

"severity.rules": { "(.*)": "advice" }

Diff Detail

Repository
rARC Arcanist
Branch
puppet-lint-respect-severity
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 6413
Build 6435: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

TimeTest
36 msArcanistXMLLinterTestCase::testLinter
28 msArcanistChmodLinterTestCase::testLinter
0 msArcanistClosureLinterTestCase::testVersion
0 msArcanistCpplintLinterTestCase::testVersion
25 msArcanistFilenameLinterTestCase::testLinter
View Full Test Results (1 Failed · 20 Passed · 31 Skipped)

Event Timeline

zmousm retitled this revision from to Let ArcanistPuppetLintLinter respect severity/severity.rules maps.
zmousm updated this object.
zmousm edited the test plan for this revision. (Show Details)
zmousm added a reviewer: epriestley.
zmousm set the repository for this revision to rARC Arcanist.
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/lint/linter/ArcanistPuppetLintLinter.php
119–137

I think that it would be simpler to use getLintMessageSeverity($matches[2]) here and provide an implementation of the getDefaultMessageSeverity method.

This revision now requires changes to proceed.May 5 2015, 8:57 AM
src/lint/linter/ArcanistPuppetLintLinter.php
123–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.

epriestley edited edge metadata.

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.
zmousm edited edge metadata.
zmousm edited the test plan for this revision. (Show Details)

Take two, following suggestions by @epriestley

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).

joshuaspence edited edge metadata.

This should wait until after T8474. Specifically, many linters suffer from this problem and T8474 should resolve this universally.

This revision now requires changes to proceed.Jun 21 2015, 9:43 PM

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.