lint: convert format strings linter test to python #24802

1. 
   Eunoia1729 commented at 6:12 pm on April 7, 2022: contributor

   Refs #24783

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

   Additional Notes(Optional):

   1. 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.
   2. 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.
   3. 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.
2. Eunoia1729 force-pushed on Apr 7, 2022
3. DrahtBot added the label Tests on Apr 7, 2022
4. Eunoia1729 force-pushed on Apr 7, 2022
5. 
   laanwj commented at 11:28 am on April 8, 2022: member

   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.

6. in test/lint/lint-format-strings.py:64 in 08de596abe outdated

   59+        run_lint_args = ['test/lint/run-lint-format-strings.py',
   60+                            '--skip-arguments', skip_arguments, function_name,] + matching_files
   61+        try:
   62+            subprocess.check_output(run_lint_args, stderr = subprocess.STDOUT)
   63+        except subprocess.CalledProcessError:
   64+            exit(1)
   

   ---

   ---

   

   laanwj commented at 11:32 am on April 8, 2022:

   nit: the convention is to use sys.exit instead of the built-in, which is only meant for interactive use.

   ---

   

   Eunoia1729 commented at 8:20 pm on April 9, 2022:

   Thank you for reviewing ! I’ve updated the PR according to your suggestions
7. Eunoia1729 force-pushed on Apr 9, 2022
8. Eunoia1729 force-pushed on Apr 9, 2022
9. in test/lint/lint-format-strings.py:3 in 879c83697d outdated

   0@@ -0,0 +1,71 @@
   1+#!/usr/bin/env python3
   2+#
   3+# Copyright (c) 2018-2020 The Bitcoin Core developers
   

   ---

   ---

   

   vincenzopalazzo commented at 11:38 pm on April 11, 2022:

   0# Copyright (c) 2018-2022 The Bitcoin Core developers
   

   ---

   

   Eunoia1729 commented at 9:35 pm on April 13, 2022:

   Thank you for the review ! I have updated the PR as per your suggestions.
10. in test/lint/lint-format-strings.py:12 in 879c83697d outdated

     7+
     8+"""
     9+Lint format strings: This program checks that the number of arguments passed
    10+to a variadic format string function matches the number of format specifiers
    11+in the format string.
    12+"""
    

    ---

    ---

    

    vincenzopalazzo commented at 11:39 pm on April 11, 2022:

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

    ---

    

    Eunoia1729 commented at 9:37 pm on April 13, 2022:

    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 ?
11. 
    vincenzopalazzo commented at 11:40 pm on April 11, 2022: none

    utACK https://github.com/bitcoin/bitcoin/pull/24802/commits/879c83697d772d31d629b885395b42686e9b1b36
12. 
    MarcoFalke commented at 7:12 am on April 12, 2022: member

    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.
13. lint: convert format strings linter test to python 267684ee34
14. Eunoia1729 force-pushed on Apr 13, 2022
15. 
    Eunoia1729 commented at 9:40 pm on April 13, 2022: contributor

    @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.

16. Eunoia1729 requested review from laanwj on Apr 13, 2022
17. 
    laanwj commented at 4:32 pm on April 25, 2022: member

    Code review ACK 267684ee34d795e4f762c4949fa45d99fe74dde3
18. laanwj merged this on Apr 25, 2022
19. laanwj closed this on Apr 25, 2022

    ---

20. in test/lint/lint-format-strings.py:39 in 267684ee34

    34+]
    35+RUN_LINT_FILE = 'test/lint/run-lint-format-strings.py'
    36+
    37+def check_doctest():
    38+    command = [
    39+        'python3',
    

    ---

    ---

    

    MarcoFalke commented at 10:48 am on April 26, 2022:

    wouldn’t it be better to use sys.executable?
21. DrahtBot locked this on Apr 26, 2023

Contributors
Eunoia1729 laanwj vincenzopalazzo MarcoFalke

Review Requested
laanwj

Labels
Tests