Integrate the string format linter with arcanist
ClosedPublic
Actions

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING6ce7c7d765f2: Integrate the string format linter with arcanist
rABC6ce7c7d765f2: Integrate the string format linter with arcanist

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

Lint Not Applicable

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 reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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.

Harbormaster completed remote builds in B4357: Diff 6413.Dec 22 2018, 19:03

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

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

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

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 summary of this revision. (Show Details)Dec 27 2018, 16:11

Fabien edited the test plan for this revision. (Show Details)

Fabien added a parent revision: D2225: Improve tests for the string format linter.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4365: Diff 6425.Dec 27 2018, 16:20

deadalnix requested changes to this revision.Dec 27 2018, 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.Dec 27 2018, 23:58

Fabien added inline comments.Dec 28 2018, 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.