lint: convert format strings linter test to python by Eunoia1729 · Pull Request #24802 · bitcoin/bitcoin

Eunoia1729 · 2022-04-07T18:12:09Z

Attempted to keep the style and flow of implementation as it is.

Additional Notes(Optional):

There is scope of improvement on how the related files are fetched. In this git grep with subprocess is still used as I found it to be the simplest. Any pointers on this are appreciated.
Removed sort operation on the matching files as I couldn't think of any strong arguments to have it. Any pointers on this are appreciated.
Not important, but one small detail is that the previous implementation was storing matched files for all the function_names iterated so far. Fixed that in this PR.

laanwj · 2022-04-08T11:28:15Z

There is scope of improvement on how the related files are fetched. In this git grep with subprocess is still used as I found it to be the simplest.

I think calling out to git grep (and git subcommands in general) is fine. The alternative would be to add a dependency on a git python module. Also, git is a single implementation on every platform (besides version differences). I've updated the guidelines in #24783 for this.

Removed sort operation on the matching files as I couldn't think of any strong arguments to have it. Any pointers on this are appreciated.

I don't know the original motivation, the only thing I can think of is that printing errors in with the files in a deterministic sorted order is somewhat-user-friendly. But I don't think it's a big deal.

test/lint/lint-format-strings.py

vincenzopalazzo

utACK 879c836

test/lint/lint-format-strings.py

vincenzopalazzo · 2022-04-11T23:39:04Z

test/lint/lint-format-strings.py

Suggested change

#!/usr/bin/env python3

#

# Copyright (c) 2018-2020 The Bitcoin Core developers

# Distributed under the MIT software license, see the accompanying

# file COPYING or http://www.opensource.org/licenses/mit-license.php.

#

"""

Lint format strings: This program checks that the number of arguments passed

to a variadic format string function matches the number of format specifiers

in the format string.

"""

#!/usr/bin/env python3

"""

Lint format strings: This program checks that the number of arguments passed

to a variadic format string function matches the number of format specifiers

in the format string.

Copyright (c) 2018-2020 The Bitcoin Core developers

Distributed under the MIT software license, see the accompanying

file COPYING or http://www.opensource.org/licenses/mit-license.php.

"""

Previously ported python scripts have the copyright comment in the way, existing version of this PR has it. Changing it to the suggested way might make it appear inconsistent with other lint files. Do you still suggest changing ?

maflcko · 2022-04-12T07:12:44Z

From reading the code it looks like all output and stderr are muted. If yes, why? If no, please provide example stdout/stderr of the test before and after this change.

Eunoia1729 · 2022-04-13T21:40:28Z

@MarcoFalke Thanks for highlighting that! That led me to update this PR so output matches with the older version.
Before change:

After change:

The reason why before version has 3 lines printed and not 2 is due to the issue highlighed in point (3) in the additional notes of PR header section. IMO the new version is the intended behavior.

laanwj · 2022-04-25T16:32:22Z

Code review ACK 267684e

maflcko · 2022-04-26T10:48:06Z

test/lint/lint-format-strings.py

+
+def check_doctest():
+    command = [
+        'python3',


wouldn't it be better to use sys.executable?

Eunoia1729 force-pushed the lint-format-strings-py branch from 396ed2c to c4f7833 Compare April 7, 2022 19:07

DrahtBot added the Tests label Apr 7, 2022

Eunoia1729 force-pushed the lint-format-strings-py branch from c4f7833 to 08de596 Compare April 7, 2022 21:32

laanwj mentioned this pull request Apr 8, 2022

Port lint scripts to Python #24783

Closed

25 tasks

laanwj reviewed Apr 8, 2022

View reviewed changes

test/lint/lint-format-strings.py Outdated Show resolved Hide resolved

laanwj mentioned this pull request Apr 8, 2022

lint: convert submodule linter test to Python #24803

Merged

Eunoia1729 force-pushed the lint-format-strings-py branch 2 times, most recently from ed2ade4 to 879c836 Compare April 9, 2022 20:44

vincenzopalazzo reviewed Apr 11, 2022

View reviewed changes

lint: convert format strings linter test to python

267684e

Eunoia1729 force-pushed the lint-format-strings-py branch from 879c836 to 267684e Compare April 13, 2022 21:32

Eunoia1729 requested a review from laanwj April 13, 2022 21:43

laanwj merged commit 0342ae1 into bitcoin:master Apr 25, 2022

maflcko reviewed Apr 26, 2022

View reviewed changes

test/lint/lint-format-strings.py

def check_doctest():

command = [

'python3',

Copy link

Member

maflcko Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be better to use sys.executable?

bitcoin locked and limited conversation to collaborators Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: convert format strings linter test to python#24802

lint: convert format strings linter test to python#24802
laanwj merged 1 commit intobitcoin:masterfrom
Eunoia1729:lint-format-strings-py

Eunoia1729 commented Apr 7, 2022 •

edited

Loading

Uh oh!

laanwj commented Apr 8, 2022 •

edited

Loading

Uh oh!

Uh oh!

vincenzopalazzo left a comment

Uh oh!

Uh oh!

vincenzopalazzo Apr 11, 2022

Uh oh!

Eunoia1729 Apr 13, 2022 •

edited

Loading

Uh oh!

maflcko commented Apr 12, 2022

Uh oh!

Eunoia1729 commented Apr 13, 2022 •

edited

Loading

Uh oh!

laanwj commented Apr 25, 2022

Uh oh!

maflcko Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-#!/usr/bin/env python3
-#
-# Copyright (c) 2018-2020 The Bitcoin Core developers
-# Distributed under the MIT software license, see the accompanying
-# file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#
-"""
-Lint format strings: This program checks that the number of arguments passed
-to a variadic format string function matches the number of format specifiers
-in the format string.
-"""
+#!/usr/bin/env python3
+"""
+Lint format strings: This program checks that the number of arguments passed
+to a variadic format string function matches the number of format specifiers
+in the format string.
+Copyright (c) 2018-2020 The Bitcoin Core developers
+Distributed under the MIT software license, see the accompanying
+file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""

Conversation

Eunoia1729 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Notes(Optional):

Uh oh!

laanwj commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincenzopalazzo Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

Eunoia1729 Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 12, 2022

Uh oh!

Eunoia1729 commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 25, 2022

Uh oh!

maflcko Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Eunoia1729 commented Apr 7, 2022 •

edited

Loading

laanwj commented Apr 8, 2022 •

edited

Loading

Eunoia1729 Apr 13, 2022 •

edited

Loading

Eunoia1729 commented Apr 13, 2022 •

edited

Loading