Page MenuHomePhabricator

Integrate the string format linter with arcanist
ClosedPublic

Authored by Fabien on Dec 22 2018, 18:52.

Details

Summary

The linter can be run with arc lint to check all the files in a diff.
The bash script now takes a single file as an argument to make it
compliant with arcanist linter expectations.

Depends on D2025 and D2225

Test Plan

Modify some files with a string format error and run arc lint.
Check that the error is catched by the linter.

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.Dec 22 2018, 18:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 22 2018, 18:52
Herald added a subscriber: schancel. · View Herald Transcript
Fabien added inline comments.Dec 22 2018, 18:56
test/lint/lint-format-strings.sh
37 ↗(On Diff #6413)

Because arcanist will check each file individually, exit should return non zero. Otherwise when a lint error occurs arcanist will stop with a script error instead of parsing the output and process the next files.

deadalnix requested changes to this revision.Dec 23 2018, 02:22
deadalnix added inline comments.
test/lint/lint-format-strings.sh
30 ↗(On Diff #6413)

Why the change ?

This revision now requires changes to proceed.Dec 23 2018, 02:22
Fabien added inline comments.Dec 23 2018, 13:16
test/lint/lint-format-strings.sh
30 ↗(On Diff #6413)

If you fail the test, why would you run a defective linter ?

Fabien requested review of this revision.Dec 23 2018, 19:04
deadalnix requested changes to this revision.Mon, Dec 24, 15:22

The test plan is not adequate. To check that linter works properly require to actually trigger an error and see that it's reported properly.

test/lint/lint-format-strings.sh
30 ↗(On Diff #6413)

Will it break something if I do?
Will is prevent to "Integrate the string format linter with arcanist"?

This revision now requires changes to proceed.Mon, Dec 24, 15:22
Fabien planned changes to this revision.Mon, Dec 24, 16:43

The linter internal tests actually check that the linter internal functions are working as expected, but I agree there is no test on the linter whole chain. I will add these tests.

test/lint/lint-format-strings.sh
30 ↗(On Diff #6413)

It will not prevent it from running with arcanist, but may eventually flood the output with false detection, making it harder to figure out that the linter itself failed.
Running the linter if the test fail will consume time to make your output more difficult to read while giving no usable information.

Fabien updated this revision to Diff 6425.Thu, Dec 27, 16:09

Rebase and update the regex to avoid change in the python script

Fabien edited the summary of this revision. (Show Details)Thu, Dec 27, 16:11
Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Thu, Dec 27, 23:58
deadalnix added inline comments.
test/lint/lint-format-strings.sh
30 ↗(On Diff #6425)

revert

35 ↗(On Diff #6425)

The exit code shouldn't be 0 if the linter failed.

38 ↗(On Diff #6425)

revert

This revision now requires changes to proceed.Thu, Dec 27, 23:58
Fabien added inline comments.Fri, Dec 28, 09:44
test/lint/lint-format-strings.sh
35 ↗(On Diff #6425)

A non zero return tells arcanist that the script failed to execute, and stop the linting process without parsing the output for linter error.
The way arcanist works expect the script to return 0 while the linter is working.

Fabien updated this revision to Diff 6431.Fri, Dec 28, 09:45

Revert exit

deadalnix accepted this revision.Sun, Dec 30, 12:04
This revision is now accepted and ready to land.Sun, Dec 30, 12:04
This revision was automatically updated to reflect the committed changes.