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
Branch
arcanist_format_linter
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4365
Build 6795: Bitcoin ABC Buildbot (legacy)
Build 6794: arc lint + arc unit

Event Timeline

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
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.Dec 24 2018, 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.Dec 24 2018, 15:22
Fabien planned changes to this revision.Dec 24 2018, 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.

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

Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 27 2018, 23:58
deadalnix added inline comments.
test/lint/lint-format-strings.sh
30

revert

35

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

38

revert

This revision now requires changes to proceed.Dec 27 2018, 23:58
test/lint/lint-format-strings.sh
35

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.

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