Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: convert format strings linter test to python #24802

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

Eunoia1729
Copy link
Contributor

@Eunoia1729 Eunoia1729 commented Apr 7, 2022

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.

@laanwj
Copy link
Member

laanwj commented Apr 8, 2022

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.

@laanwj laanwj mentioned this pull request Apr 8, 2022
25 tasks
@Eunoia1729 Eunoia1729 force-pushed the lint-format-strings-py branch 2 times, most recently from ed2ade4 to 879c836 Compare April 9, 2022 20:44
Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

utACK 879c836

test/lint/lint-format-strings.py Outdated Show resolved Hide resolved
Comment on lines 1 to 12
#!/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.
"""

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Eunoia1729 Eunoia1729 Apr 13, 2022

Choose a reason for hiding this comment

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

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
Copy link
Member

maflcko commented Apr 12, 2022

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
Copy link
Contributor Author

Eunoia1729 commented Apr 13, 2022

@MarcoFalke Thanks for highlighting that! That led me to update this PR so output matches with the older version.
Before change:
Screenshot from 2022-04-14 02-36-54

After change:
Screenshot from 2022-04-14 02-54-53

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.

@Eunoia1729 Eunoia1729 requested a review from laanwj April 13, 2022 21:43
@laanwj
Copy link
Member

laanwj commented Apr 25, 2022

Code review ACK 267684e

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

def check_doctest():
command = [
'python3',
Copy link
Member

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 bitcoin locked and limited conversation to collaborators Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants