Page MenuHomePhabricator

ArcanistPyLintLinter.php has superfluous quotations around the "msg-template" options.
Closed, ResolvedPublic

Description

Hey everyone, thanks very much for offering support for Pylint. I was trying to use the new linter out and encountered this error:

>>> [16] <exec> $ "pylint" "--reports=no" "--msg-template= {line}|{column}|{msg_id}|{symbol}|{msg} " "C:\codes\armi\setup.py"
<<< [12] <lint> 680,114 us
>>> [17] <lint> PyLint <paths = 1>
<<< [16] <exec> 720,123 us
<<< [17] <lint> 720,123 us

[2015-08-18 10:57:29] EXCEPTION: (PhutilAggregateException) Some linters failed:
    - Exception: Parameter passed to "setLine()" must be an integer. at [<arcanist>\src\lint\engine\ArcanistLintEngine.php:273]
arcanist(head=master, ref.master=fe8ed2a6f8b0), phutil(head=master, ref.master=e509fc30ae78)
  #0 <#2> ArcanistLintMessage::validateInteger(string, string) called at [<arcanist>\src\lint\ArcanistLintMessage.php:75]
  #1 <#2> ArcanistLintMessage::setLine(string) called at [<arcanist>\src\lint\linter\ArcanistPyLintLinter.php:135]
  #2 <#2> ArcanistPyLintLinter::parseLinterOutput(string, integer, string, string) called at [<arcanist>\src\lint\linter\ArcanistExternalLinter.php:355]
  #3 <#2> ArcanistExternalLinter::resolveFuture(string, ExecFuture) called at [<arcanist>\src\lint\linter\ArcanistFutureLinter.php:34]
  #4 <#2> ArcanistFutureLinter::didLintPaths(array) called at [<arcanist>\src\lint\engine\ArcanistLintEngine.php:600]
  #5 <#2> ArcanistLintEngine::executeDidLintOnPaths(ArcanistPyLintLinter, array) called at [<arcanist>\src\lint\engine\ArcanistLintEngine.php:551]
  #6 <#2> ArcanistLintEngine::executeLintersOnChunk(array, array) called at [<arcanist>\src\lint\engine\ArcanistLintEngine.php:479]
  #7 <#2> ArcanistLintEngine::executeLinters(array) called at [<arcanist>\src\lint\engine\ArcanistLintEngine.php:217]
  #8 ArcanistLintEngine::run() called at [<arcanist>\src\workflow\ArcanistLintWorkflow.php:336]
  #9 ArcanistLintWorkflow::run() called at [<arcanist>\scripts\arcanist.php:382]

I didn't see any relevant tickets about pylint with a few searches here so forgive me if I missed an already existing discussion.

I went poking about and noticed the regular expression match in <arcanist>\src\lint\ArcanistLintMessage.php wasn't operating on the input correctly:

// Strings like "234" are fine, coerce them to integers.
if (is_string($value) && preg_match('/^\d+\z/', $value)) {
  $value = (int)$value;
}

I looked at some input and saw " 53" with the leading space. Changing the regex to '/^\s*\d+\z/' confirmed this and the linting continued on, operating correctly.

On a bit of a guess I noticed that the source of the input "--msg-template= {line}|{column}|{msg_id}|{symbol}|{msg} " has extra spacing. Trying out the following before/after code in <src>\src\lint\linter\ArcanistPyLintLinter.php and removing the regex change, things still worked.

-    $options[] = '--msg-template="{line}|{column}|{msg_id}|{symbol}|{msg}"';
+    $options[] = '--msg-template={line}|{column}|{msg_id}|{symbol}|{msg}';

So, I'm guessing the quotations were being replaced with spacing to not break the enclosing quotations on the msg-template. If I'm right, would you like me to contribute to the code base after running some tests? or have some more veteran contributor remove the possibly excess quotations? I'm not sure if this is a problem that only occurs in my environment, but for what it's worth I updated arcanist, libphutil, and pylint to the latest.

Thanks again!

Event Timeline

e-m-albright updated the task description. (Show Details)
e-m-albright added projects: Arcanist, Lint.
e-m-albright updated the task description. (Show Details)
e-m-albright added a subscriber: e-m-albright.

I think this change is correct and the analysis backing it up looks accurate.

I have no clue how to install pylint or any of the things which install pylint to actually test it, and I don't really want to install any of the things which install the things which install pylint because my system currently works properly.

@joshuaspence, do you happen to have pylint working locally? My recollection is that this change is safe after we made all the flags always get quoted, and that appears to be backed up by tracing the source.

If you want to arc diff this I'll accept it as soon as anyone else confirms it works, or I manage to install pylint, or we write a package management system capable of installing pylint (see T8117, etc).

epriestley claimed this task.

Presuming resolved since D13931 is in HEAD. Thanks for the report!

Yep, thanks much for your assistance. I appreciate the product!