test: Move linters to test/lint, add readme #13281

maflcko · 2018-05-19T14:49:20Z

This moves the checks and linters from devtools to a subfolder in test. (Motivated by my opinion that the dev tools are mostly for generating code and updating the repo whereas the linters are read-only checks.)

Also, adds a readme to clarify that checks and linters are only meant to prevent bugs and user facing issues, not merely stylistic preference or inconsistencies. (This is motivated by the diversity in developers and work flows as well as existing code styles. It would be too disruptive to change all existing code to a single style or too burdensome to force all developers to adhere to a single style. Also note that our style guide is changing, so locking in at the wrong style "too early" would only waste resources.)

practicalswift · 2018-05-21T08:00:41Z

The test/lint/ move is an obvious utACK, but the "add readme" part is troublesome:

The following addition …

This folder contains lint scripts that aim to prevent direct or indirect user facing bugs.
Patches that do not pass the linter checks are usually not accepted.

Note that the [recommended style](/doc/developer-notes.md) and other stylistic
inconsistencies should not be enforced unless they could result in user facing bugs.

… formalizes a new "automation skeptical" (for lack of a better word!) policy which in the light of recent discussions on linting (see for example the discussion in @kallewoof's PR #12879) does not seem to be backed by consensus.

Personally I subscribe to the automation/linting philosophy outlined by @sipa in #12879:

If there is a widespread opinion among contributors and maintainers that a particul guideline is worth the burden, then I think it should be put in the style guide - and if it is, it sounds great to enforce it.

I suggest the following text instead:

This folder contains lint scripts that automate the review process in some form.

If there is a widespread opinion among contributors and maintainers that a particular
guideline is worth the burden, then it should be put in the style guide - and if it is,
it is probably a good candidate for linting. However, the enforcement via linting should
be limited to issues that can reliably identified without false positives or false
negatives.

Patches that do not pass the linter checks are usually not accepted.

maflcko · 2018-05-21T14:14:21Z

@practicalswift That was my first draft, but I realized that the style guide includes not only enforceable guidelines, but also a bunch of recommendations, that are fine to violate. For example the "Miscellaneous" coding style or the reference to the clang-format rules. While most of them can be reliably identified without false positives or false negatives, I think we should avoid enforcing them.

maflcko · 2018-05-23T21:10:54Z

Also note that the wording is not strict "[...] other stylistic inconsistencies should not be enforced [...]"

We can always add a linter for stuff that is uncontroversial but not preventing any user facing bugs. See for example the linter that checks for trailing white space.

promag · 2018-05-23T22:48:12Z

test/liny/lint-python.sh also catches python scripts in contrib/ and share/ - fine I guess?

maflcko · 2018-05-24T14:27:42Z

@promag I guess that is fine or maybe even desired.

jnewbery · 2018-05-24T15:36:57Z

Conecpt ACK moving the linters to test/lint. No need IMO to bundle that change with a new readme setting a policy about what should be linted.

If you want to change the linter policy, then it seems appropriate to raise it in a meeting. For this PR, I recommend you limit it to just moving the files.

maflcko · 2018-05-24T15:59:16Z

@practicalswift @jnewbery Done

practicalswift · 2018-05-24T16:24:42Z

@MarcoFalke Looks good! :-)

utACK fa3c910

sipa · 2018-05-24T18:31:12Z

utACK fa3c910

kallewoof · 2018-05-25T04:16:47Z

utACK fa3c910

Empact · 2018-05-25T10:22:06Z

contrib/devtools/README.md still documents check-doc, which has moved.

jnewbery · 2018-05-25T13:51:32Z

utACK fa3c910 , modulo Empact's comment about the check-doc documentation.

ken2812221 · 2018-05-28T19:07:57Z

utACK fa3c910

laanwj · 2018-05-29T13:45:39Z

I'm just going to merge this, so many utACKs for the current state.

fa3c910 test: Move linters to test/lint, add readme (MarcoFalke) Pull request description: This moves the checks and linters from `devtools` to a subfolder in `test`. (Motivated by my opinion that the dev tools are mostly for generating code and updating the repo whereas the linters are read-only checks.) Also, adds a readme to clarify that checks and linters are only meant to prevent bugs and user facing issues, not merely stylistic preference or inconsistencies. (This is motivated by the diversity in developers and work flows as well as existing code styles. It would be too disruptive to change all existing code to a single style or too burdensome to force all developers to adhere to a single style. Also note that our style guide is changing, so locking in at the wrong style "too early" would only waste resources.) Tree-SHA512: 9b10e89f2aeaf0c8a9ae248aa891d74e0abf0569f8e5dfd266446efa8bfaf19f0ea0980abf0b0b22f0d8416ee90d7435d21a9f9285b66df43f370b7979173406

93843f6 doc: remove leftover check-doc documentation (fanquake) Pull request description: Remove leftover check-doc.py documentation. Mentioned [here](#13281 (comment)), it's now [here](https://github.com/bitcoin/bitcoin/tree/master/test/lint#check-docpy). Tree-SHA512: 95a0ac221ffae109c1d4baf18a9220cf993fc07c005920a0bd09abdf52e8fb298e3b5df31fa18887719c5080d8531d18b84b7bd9c7c664ee2501ccd9e0975eb6

14b29a7 Fix reference to lint-locale-dependence.sh (Hennadii Stepanov) Pull request description: The wrong reference sneaked through bitcoin#13041 and bitcoin#13281. Tree-SHA512: 39cc74297d00141206ce68b84575288a20e39e20ef717fa8b8c09076b5fb46d8281320b144a596094365f2a0704e5dd6bf960e35980ae4730546a72957403d69

93843f6 doc: remove leftover check-doc documentation (fanquake) Pull request description: Remove leftover check-doc.py documentation. Mentioned [here](bitcoin#13281 (comment)), it's now [here](https://github.com/bitcoin/bitcoin/tree/master/test/lint#check-docpy). Tree-SHA512: 95a0ac221ffae109c1d4baf18a9220cf993fc07c005920a0bd09abdf52e8fb298e3b5df31fa18887719c5080d8531d18b84b7bd9c7c664ee2501ccd9e0975eb6

fa3c910 test: Move linters to test/lint, add readme (MarcoFalke) Pull request description: This moves the checks and linters from `devtools` to a subfolder in `test`. (Motivated by my opinion that the dev tools are mostly for generating code and updating the repo whereas the linters are read-only checks.) Also, adds a readme to clarify that checks and linters are only meant to prevent bugs and user facing issues, not merely stylistic preference or inconsistencies. (This is motivated by the diversity in developers and work flows as well as existing code styles. It would be too disruptive to change all existing code to a single style or too burdensome to force all developers to adhere to a single style. Also note that our style guide is changing, so locking in at the wrong style "too early" would only waste resources.) Tree-SHA512: 9b10e89f2aeaf0c8a9ae248aa891d74e0abf0569f8e5dfd266446efa8bfaf19f0ea0980abf0b0b22f0d8416ee90d7435d21a9f9285b66df43f370b7979173406

Verifier for scriptable changes Includes changes from the following upstream PRs: - bitcoin/bitcoin#10189 - Excluding the `CNode` scripted changes. - bitcoin/bitcoin#10480 - bitcoin/bitcoin#11390 - bitcoin/bitcoin#13281 - Only the lint scripts we already have. - bitcoin/bitcoin#13454 - Only changes to scripts we already have. - bitcoin/bitcoin#14864 - bitcoin/bitcoin#16327 - Includes some portability fixes to other shell scripts. - bitcoin/bitcoin#20069

14b29a7 Fix reference to lint-locale-dependence.sh (Hennadii Stepanov) Pull request description: The wrong reference sneaked through bitcoin#13041 and bitcoin#13281. Tree-SHA512: 39cc74297d00141206ce68b84575288a20e39e20ef717fa8b8c09076b5fb46d8281320b144a596094365f2a0704e5dd6bf960e35980ae4730546a72957403d69

maflcko force-pushed the Mf1805-qaLint branch 3 times, most recently from 8f00208 to 920b523 Compare May 19, 2018 14:54

laanwj added Refactoring Tests labels May 19, 2018

maflcko force-pushed the Mf1805-qaLint branch 2 times, most recently from 535e512 to 288434d Compare May 24, 2018 15:58

test: Move linters to test/lint, add readme

fa3c910

maflcko force-pushed the Mf1805-qaLint branch from 288434d to fa3c910 Compare May 24, 2018 16:02

laanwj self-assigned this May 28, 2018

laanwj merged commit fa3c910 into bitcoin:master May 29, 2018

fanquake mentioned this pull request May 29, 2018

doc: remove leftover check-doc documentation #13340

Merged

maflcko deleted the Mf1805-qaLint branch May 29, 2018 14:11

Bushstar mentioned this pull request Jun 1, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#340

Merged

hebasto mentioned this pull request Sep 13, 2018

Docs: Fix reference to lint-locale-dependence.sh #14213

Merged

This was referenced Oct 26, 2020

Replace BOOST_STATIC_ASSERT with static_assert zcash/zcash#4826

Merged

Verifier for scriptable changes zcash/zcash#4827

Merged

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Move linters to test/lint, add readme #13281

test: Move linters to test/lint, add readme #13281

Uh oh!

maflcko commented May 19, 2018

Uh oh!

practicalswift commented May 21, 2018 •

edited

Loading

Uh oh!

maflcko commented May 21, 2018

Uh oh!

maflcko commented May 23, 2018

Uh oh!

promag commented May 23, 2018

Uh oh!

maflcko commented May 24, 2018

Uh oh!

jnewbery commented May 24, 2018

Uh oh!

maflcko commented May 24, 2018

Uh oh!

practicalswift commented May 24, 2018

Uh oh!

sipa commented May 24, 2018

Uh oh!

kallewoof commented May 25, 2018

Uh oh!

Empact commented May 25, 2018

Uh oh!

jnewbery commented May 25, 2018 •

edited

Loading

Uh oh!

ken2812221 commented May 28, 2018

Uh oh!

laanwj commented May 29, 2018

Uh oh!

Uh oh!

test: Move linters to test/lint, add readme #13281

test: Move linters to test/lint, add readme #13281

Uh oh!

Conversation

maflcko commented May 19, 2018

Uh oh!

practicalswift commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 21, 2018

Uh oh!

maflcko commented May 23, 2018

Uh oh!

promag commented May 23, 2018

Uh oh!

maflcko commented May 24, 2018

Uh oh!

jnewbery commented May 24, 2018

Uh oh!

maflcko commented May 24, 2018

Uh oh!

practicalswift commented May 24, 2018

Uh oh!

sipa commented May 24, 2018

Uh oh!

kallewoof commented May 25, 2018

Uh oh!

Empact commented May 25, 2018

Uh oh!

jnewbery commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented May 28, 2018

Uh oh!

laanwj commented May 29, 2018

Uh oh!

Uh oh!

practicalswift commented May 21, 2018 •

edited

Loading

jnewbery commented May 25, 2018 •

edited

Loading