Page MenuHomePhabricator

[LINTER] Make the check-doc linter run only once
AbandonedPublic

Authored by Fabien on Jun 14 2019, 15:16.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This linter parses the whole tree at each call, it is unnecessary to run
it for each file being modified.

This diffs adds 2 abstract classes: ArcanistGlobalLinter and
ArcanistGlobalExternalLinter.
The ArcanistGlobalLinter is an adapted version of the
ArcanistFutureLinter, and the ArcanistGlobalExternalLinter is mostly
a copy of the ArcanistExternalLinter.
They ensure that the external linter get only called once, independently
of the number of modified files.
This allow the linters that need to run a single time to simply extend
ArcanistGlobalExternalLinter instead of ArcanistExternalLinter while
keeping the same interfaces and features.

Depends on D3334
Superseedes D3300

Test Plan
arc lint --everything --trace 2>&1 | grep -E "exec.*check-doc.py'$"

Should return a single line (the only call to the check-doc.py script).

Diff Detail

Repository
rABC Bitcoin ABC
Branch
linter_check_doc_run_once
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6478
Build 11003: Bitcoin ABC Buildbot (legacy)
Build 11002: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jun 16 2019, 01:58

I'll need more time to actually review this, but it's clear that the API is not what you'd expect.

arcanist/linter/CheckDocLinter.php
59 ↗(On Diff #9444)

A global linter shouldn't depend on path, so clearly there is something wrong with the design here.

This revision now requires changes to proceed.Jun 16 2019, 01:58

Remove useless path from the global linter interface.

deadalnix requested changes to this revision.Jun 25 2019, 23:26
deadalnix added inline comments.
arcanist/linter/ArcanistGlobalExternalLinter.php
7

This is not even landed that this is already wrong. This is what code duplication is about.

552

This clearly creates massive duplication. This is bad.

It should be possible to leverage existing infrastructure.

arcanist/linter/ArcanistGlobalLinter.php
6

If ever you need a non external one time global linter, you can just create a trait to share the code. This is much better than duplicating all of the ArcanistExternalLinter.

This revision now requires changes to proceed.Jun 25 2019, 23:26