Page MenuHomePhabricator

[LINTER] Integrate lint-locale-dependence.sh into arcanist
ClosedPublic

Authored by Fabien on Mar 19 2019, 09:12.

Details

Summary

Change the linter to accept a file as an argument, and always return 0
to avoid arcanist to throw a script execution error.
Add a php class to interpret the linter output and display linting error
messages through arc lint.

Depends on D2704

Test Plan
arc lint

Comment any of the known exception in the linter-locale-dependence.sh
file and add a comment in the corresponding source file. Check that `arc
lint` displays errors for the source file.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Mar 19 2019, 09:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 19 2019, 09:12
Herald added a subscriber: schancel. · View Herald Transcript
Fabien updated this revision to Diff 7743.Mar 19 2019, 10:58

Rebase

jasonbcox requested changes to this revision.Mar 22 2019, 22:02
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
arcanist/linter/LocalDependenceLinter.php
57 ↗(On Diff #7743)

Require -> Requires

73 ↗(On Diff #7743)

<funtion> -> <function>

80 ↗(On Diff #7743)

theses -> these

test/lint/lint-locale-dependence.sh
227 ↗(On Diff #7743)

The php script indicates that the linter throws errors, but why is it that the script always returns success?

This revision now requires changes to proceed.Mar 22 2019, 22:02
Fabien added inline comments.Mar 23 2019, 08:58
test/lint/lint-locale-dependence.sh
227 ↗(On Diff #7743)

This is due to the way arcanist handle the error codes.
If the script returns non zero, arcanist consider that it failed to run and so the linter output should not be parsed.
Errors in the php file refer to the linting errors which are printed on stdout and then parrsed by the php script.
"Error" might not be the best word here, any suggestion ?

Fabien updated this revision to Diff 7810.Mar 25 2019, 08:57

Fix typos

jasonbcox accepted this revision.Mar 25 2019, 17:55
jasonbcox added inline comments.
test/lint/lint-locale-dependence.sh
227 ↗(On Diff #7743)

I think just differentiating between "linting errors" and "script errors" would be helpful enough. Thanks for the explanation.

This revision is now accepted and ready to land.Mar 25 2019, 17:55
Fabien updated this revision to Diff 7832.Tue, Mar 26, 11:11

Rebase and improve comments/summary to clarify script errors vs linting errors

Fabien edited the summary of this revision. (Show Details)Tue, Mar 26, 11:12
This revision was automatically updated to reflect the committed changes.