Page MenuHomePhabricator

Integrates the check-doc.py script to arcanist
ClosedPublic

Authored by Fabien on Jan 14 2019, 12:26.

Details

Summary

Running arc lint or arc diff will run the script.
An error will be returned for each undocumented or unknown argument.

Depends on D2306

Test Plan
arc lint

Check there is no error

Add this line to src/init.cpp (don't forget to save):

// GetArg("-dummy", "default");
arc lint

Check it outputs an undocumented argument error for -dummy
Remove the line from src/init.cpp

Add this line to src/init.cpp (don't forget to save):

// HelpMessageOpt("-dummy", "Dummy documentation");
arc lint

Check it outputs an unknown argument error for -dummy
Remove the line from src/init.cpp

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

arcanist/linter/CheckDocLinter.php
39 ↗(On Diff #6641)

You probably can return the path directly without using a local variable :)

test/lint/check-doc.py
79 ↗(On Diff #6641)

Retunring a non zero error code when problems are found seems like a good idea to me.

deadalnix requested changes to this revision.Jan 14 2019, 17:07

You need to be updating the release doc with this.

This revision now requires changes to proceed.Jan 14 2019, 17:07
test/lint/check-doc.py
79 ↗(On Diff #6641)

Yes, but this is the same problem encountered with the previous linter. If the script returns non zero, arcanist interprets as "the script did not run correctly" instead of "some linter errors were found".
Arcanist expects the linter to output to stdout and returns 0 if the output should be parsed; otherwise it throws an exception.

Remove the check in the release process document, no longer necessary
Avoid some local variables

deadalnix requested changes to this revision.Jan 15 2019, 11:39
deadalnix added inline comments.
arcanist/linter/CheckDocLinter.php
64 ↗(On Diff #6663)

Maybe arcanist consider the linter to have failed to run when it returns an error code because this is what you programmed it to do here ?

doc/release-process.md
15 ↗(On Diff #6663)

It's probably a good idea to run arc lint --everything

This revision now requires changes to proceed.Jan 15 2019, 11:39
arcanist/linter/CheckDocLinter.php
64 ↗(On Diff #6663)

Unfortunately not, this is the default behavior of arcanist (I should remove these lines, they are redondant).
Even if you enforce :

if($err != 0) {
  $err = 0;
}

, arcanist will throw an exception anyway.
It actually makes sense if you think it as a linter which is designed to get its output parsed. How would arcanist know that it's parsing a correct output otherwise ?

Add an instruction to run arc lint --everything in the release process document
Remove the check against error in the PHP parsing function, as it is redundant with arcanist default behavior

This revision is now accepted and ready to land.Jan 15 2019, 12:50
arcanist/linter/CheckDocLinter.php
63 ↗(On Diff #6671)

It's probably better to keep the check then, to make sure you don't try to parse garbage.

This revision was automatically updated to reflect the committed changes.