Update code style #31

Pennycook · 2024-01-08T21:21:18Z

Hopefully, the changes here will prepare us to enable the new hooks introduced by #21 to address #18.

There are a lot of changes here, but I've tried to break things out into separate commits in a way that will simplify the review process. I recommend reviewing the changes commit-by-commit:

The first few commits make some changes to the pre-commit setup, to address weird things I saw during the style update.
Most of the remaining commits address one linter error each, across all files.

The final commit is a final fix I implemented today resulting from the changes merged in #29.

We use print statements for console output, so shouldn't run flake8-print. We run black as part of pre-commit already, so don't need to run flake8-black. We can safely ignore the following flake8 checks: - S105: We don't use any passwords, so don't need to check for them. - E203: flake8 and black disagree on how to format [x : y] Signed-off-by: John Pennycook <[email protected]>

black and isort both require explicit line lengths for compatibility with flake8's default length of 79 characters. isort additionally needs to be told to use black-compatible formatting. Signed-off-by: John Pennycook <[email protected]>

With the original order, it was possible for files to fail the pre-commit multiple times. For example, running black may introduce a new multi-line statement that add-trailing-comma couldn't adjust (because it ran first). The new order tries to group hooks to update: 1) Formatting-independent syntax 2) Statement order 3) Formatting 4) Formatting-dependent syntax Signed-off-by: John Pennycook <[email protected]>

This commit enforces the new formatting, but does nothing to address any of the errors raised by the new linter. Signed-off-by: John Pennycook <[email protected]>

F401 Signed-off-by: John Pennycook <[email protected]>

E741 Signed-off-by: John Pennycook <[email protected]>

F541 Signed-off-by: John Pennycook <[email protected]>

F821 Signed-off-by: John Pennycook <[email protected]>

E402 Signed-off-by: John Pennycook <[email protected]>

E721 Signed-off-by: John Pennycook <[email protected]>

FS001 Signed-off-by: John Pennycook <[email protected]>

FS002 Signed-off-by: John Pennycook <[email protected]>

E501 Signed-off-by: John Pennycook <[email protected]>

Signed-off-by: John Pennycook <[email protected]>

F401 is 'Module imported but unused'. Ignoring F401 for all __init__.py files seems like a cleaner solution than introducing unnecessary references to the modules (e.g. via __all__ or similar). Signed-off-by: John Pennycook <[email protected]>

.pre-commit-config.yaml

laserkelvin · 2024-01-08T21:52:20Z

codebasin.py

@@ -47,46 +32,91 @@ def guess_project_name(config_path):
    """
    fullpath = os.path.realpath(config_path)
    (thedir, thename) = os.path.split(fullpath)
-    if config_path == 'config.yaml':
+    if config_path == "config.yaml":
        (base, end) = os.path.split(thedir)
        res = end.strip()
    else:
        (base, end) = os.path.splitext(thename)
        res = base.strip()
    if not res:


Not part of this PR, but not too much of a fan of how this is written: I think it's meant to trigger when nothing is left after you strip either end or base, but the if not ... pattern reads to me like it expects None to be returned.

I know that empty strings are treated as False in Python, but I would recommend making the intention to check more explicit for the reader with if not bool(res):, which casts a string into True if it's not empty, and False if it is.

I agree we should tidy this up.

codebasin/file_parser.py

codebasin/report.py

laserkelvin · 2024-01-08T22:13:44Z

codebasin/walkers/tree_mapper.py


-log = logging.getLogger('codebasin')
+log = logging.getLogger("codebasin")


Is this even used?

As discussed offline, this might (although it's very unlikely) be something that somebody is relying on.

I double-checked, and as far as I can tell, none of these logging statements were introduced as new code -- in some cases they got reordered, and in all cases they moved from ' to ", so the fact they were there before might not be obvious from the diff.

I think we should leave them for now, and remove them in 2.0.0.

laserkelvin · 2024-01-08T22:15:25Z

codebasin/walkers/tree_printer.py

-        spacing = '  ' * (level)
-        print('{}{} -- Platforms: {}'.format(spacing, node, platform))
+        spacing = "  " * (level)
+        print(f"{spacing}{node} -- Platforms: {platform}")


Is this print statement intentional? I think we discussed offline that you actually did intend to print as a means of getting the user's attention, in contrast to logging.

Strictly speaking I guess the "best practice" would be to add a StreamHandler to log, but I'm okay with this being a one off thing.

I actually can't find any usage of TreePrinter at all... Can you double-check?

Either way, I think this is probably something that we would want to remove in 2.0.0. I think it has to wait for a major release -- as tempted as I am to say that our "public API" is actually the command-line interface, it's theoretically possible that somebody somewhere is importing the codebasin package and then using this class.

laserkelvin · 2024-01-08T22:15:49Z

codebasin/walkers/tree_walker.py


-log = logging.getLogger('codebasin')
+log = logging.getLogger("codebasin")


Same resolution as #31 (comment).

laserkelvin · 2024-01-08T22:18:04Z

etc/sloc_translate.py

        walk_sloc(args[1], cleaned, verbose=True)
    else:
-        print("Expected either 1 argument (a single file to parse" +
-              " and print) or 2 (a directory root & file pattern)")
+        print(


Should this really be a print, warning, or exception?

I think it should really be an error, but I'm reluctant to spend time fixing this script. The etc/ directory is really a dumping ground for experimental functionality, and I'd like to remove it entirely in a future release.

This script doesn't currently import any of the logging stuff (and isn't called in a way that would configure the logging correctly) so fixing this properly would affect more than just this line. If we decide that a SLOC-counting tool is important, I'd rather reintroduce the functionality as a new script in bin/ that is actually documented, tested, and part of the package.

Signed-off-by: John Pennycook <[email protected]>

laserkelvin

I think we've addressed everything; leaving some things as "unresolved" so that we don't forget the discussions for later updates.

Pennycook added 16 commits January 8, 2024 12:41

Add compatibility options to pre-commit hooks

fe65833

black and isort both require explicit line lengths for compatibility with flake8's default length of 79 characters. isort additionally needs to be told to use black-compatible formatting. Signed-off-by: John Pennycook <[email protected]>

Run new pre-commit hooks on all files

ccebb31

This commit enforces the new formatting, but does nothing to address any of the errors raised by the new linter. Signed-off-by: John Pennycook <[email protected]>

Remove imported but unused modules

26b7b79

F401 Signed-off-by: John Pennycook <[email protected]>

Rename variables with ambiguous names

55b488c

E741 Signed-off-by: John Pennycook <[email protected]>

Fix f-string with missing placeholders

cf9544e

F541 Signed-off-by: John Pennycook <[email protected]>

Fix reference to undefined name "ParserError"

adbf6b2

F821 Signed-off-by: John Pennycook <[email protected]>

Move module level imports to top of file

d3a1768

E402 Signed-off-by: John Pennycook <[email protected]>

Use isinstance instead of comparing types

2631149

E721 Signed-off-by: John Pennycook <[email protected]>

Replace '%' operator with f-strings

bacdf2d

FS001 Signed-off-by: John Pennycook <[email protected]>

Replace usage of '.format' with f-strings

6765eaa

FS002 Signed-off-by: John Pennycook <[email protected]>

Manually wrap some lines to 79 characters

5300ba5

E501 Signed-off-by: John Pennycook <[email protected]>

Remove comments used to disable old hooks

df852a5

Signed-off-by: John Pennycook <[email protected]>

Bump copyright to 2024

2e3c66f

Signed-off-by: John Pennycook <[email protected]>

Ignore F401 in __init__.py files

ca465f2

F401 is 'Module imported but unused'. Ignoring F401 for all __init__.py files seems like a cleaner solution than introducing unnecessary references to the modules (e.g. via __all__ or similar). Signed-off-by: John Pennycook <[email protected]>

Pennycook requested a review from laserkelvin January 8, 2024 21:21

laserkelvin requested changes Jan 8, 2024

View reviewed changes

Use matplotlib functions consistently

3d55e51

Signed-off-by: John Pennycook <[email protected]>

laserkelvin approved these changes Jan 9, 2024

View reviewed changes

Pennycook merged commit 77b56e5 into intel:main Jan 9, 2024

Pennycook deleted the update-code-style branch January 9, 2024 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update code style #31

Update code style #31

Pennycook commented Jan 8, 2024

laserkelvin Jan 8, 2024

Pennycook Jan 8, 2024

laserkelvin Jan 8, 2024

Pennycook Jan 9, 2024

laserkelvin Jan 8, 2024

Pennycook Jan 8, 2024

laserkelvin Jan 8, 2024

Pennycook Jan 9, 2024

laserkelvin Jan 8, 2024

Pennycook Jan 8, 2024

laserkelvin left a comment


		log = logging.getLogger('codebasin')
		log = logging.getLogger("codebasin")

Update code style #31

Update code style #31

Conversation

Pennycook commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laserkelvin left a comment

Choose a reason for hiding this comment