Auto-format Python sources with black, fix all issues reported by flake8 by nucleogenic · Pull Request #1010 · PiSCSI/piscsi

nucleogenic · 2022-11-29T23:06:24Z

Auto-formatted all Python sources with black.

Fixed all issues reported by flake8.

Due to the config files moving up a level (web -> python), these commands should now be run from the python directory.

Checks in GitHub Actions will be updated in PR #1009.

WIP WIP WIP

rdmark

The end result is a overwhelming net positive for readability and maintainability. I found a small minority of cases where black condensed nested code into one liners with decreased readability. I wonder if there is some kind of tweak to the rules we can do here?

rdmark · 2022-11-30T02:10:58Z

python/common/src/rascsi/file_cmds.py

-                    for file in files
-                ]
-            )
+            files_list.extend([(file, path.getsize(path.join(file_path, file))) for file in files])


I find it really hard to read the code like this. Any chance we can tweak this rule (if there is one)?

From what I've read, untangling comprehensions based on a "complexity" heuristic is still a WIP in Black.

I was able to get it to:

files_list.extend( [(file, path.getsize(path.join(file_path, file))) for file in files], )

.... which is a small improvement.

Failing that, a simpler refactor might be possible.

Fair point, this mess should better be refactored anyways. Do we want to add a TODO? Totally optional in this PR but just a thought so that it isn't forgotten.

rdmark · 2022-11-30T02:19:29Z

python/ctrlboard/src/ctrlboard_menu_builder.py

+        menu.add_entry("Return", {"context": self.ACTION_MENU, "action": self.ACTION_RETURN})
+        menu.add_entry(
+            "Attach/Insert",
+            {"context": self.ACTION_MENU, "action": self.ACTION_SLOT_ATTACHINSERT},
+        )
+        menu.add_entry(
+            "Detach/Eject",
+            {"context": self.ACTION_MENU, "action": self.ACTION_SLOT_DETACHEJECT},
+        )
+        menu.add_entry("Info", {"context": self.ACTION_MENU, "action": self.ACTION_SLOT_INFO})
+        menu.add_entry(
+            "Load Profile",
+            {"context": self.ACTION_MENU, "action": self.ACTION_LOADPROFILE},
+        )
+        menu.add_entry("Shutdown", {"context": self.ACTION_MENU, "action": self.ACTION_SHUTDOWN})


Nitpick: I don't like how each method call uses different formatting. Would be easier to read if they were all consistent.

Fixed by adding a trailing comma before the final ), e.g.

menu.add_entry( "Return", {"context": self.ACTION_MENU, "action": self.ACTION_RETURN}, ) menu.add_entry( "Attach/Insert", {"context": self.ACTION_MENU, "action": self.ACTION_SLOT_ATTACHINSERT}, )

Further commentary on this approach is here:
https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#pragmatism

python/pyproject.toml

rdmark · 2022-11-30T02:28:54Z

python/web/src/return_code_mapper.py

+        ReturnCodes.DELETEFILE_SUCCESS: _("File deleted: %(file_path)s"),
+        ReturnCodes.DELETEFILE_FILE_NOT_FOUND: _("File to delete not found: %(file_path)s"),
+        ReturnCodes.RENAMEFILE_SUCCESS: _("File moved to: %(target_path)s"),
+        ReturnCodes.RENAMEFILE_UNABLE_TO_MOVE: _("Unable to move file to: %(target_path)s"),
+        ReturnCodes.DOWNLOADFILETOISO_SUCCESS: _(
+            'Created CD-ROM ISO image with arguments "%(value)s"'
+        ),
+        ReturnCodes.DOWNLOADTODIR_SUCCESS: _("%(file_name)s downloaded to %(save_dir)s"),
+        ReturnCodes.WRITEFILE_SUCCESS: _("File created: %(target_path)s"),
+        ReturnCodes.WRITEFILE_COULD_NOT_WRITE: _("Could not create file: %(target_path)s"),
+        ReturnCodes.READCONFIG_SUCCESS: _("Loaded configurations from: %(file_name)s"),
+        ReturnCodes.READCONFIG_COULD_NOT_READ: _(
+            "Could not read configuration file: %(file_name)s"
+        ),
+        ReturnCodes.READCONFIG_INVALID_CONFIG_FILE_FORMAT: _("Invalid configuration file format"),
+        ReturnCodes.READDRIVEPROPS_SUCCESS: _("Read properties from file: %(file_path)s"),
+        ReturnCodes.READDRIVEPROPS_COULD_NOT_READ: _(
+            "Could not read properties from file: %(file_path)s"
+        ),
+        ReturnCodes.ATTACHIMAGE_COULD_NOT_ATTACH: _(
+            "Cannot insert an image for %(device_type)s into a %(current_device_type)s device"
+        ),
+        ReturnCodes.EXTRACTIMAGE_SUCCESS: _("Extracted %(count)s file(s)"),
+        ReturnCodes.EXTRACTIMAGE_NO_FILES_SPECIFIED: _(
+            "Unable to extract archive: No files were specified"
+        ),
+        ReturnCodes.EXTRACTIMAGE_NO_FILES_EXTRACTED: _(
+            "No files were extracted (existing files are skipped)"
+        ),
+        ReturnCodes.EXTRACTIMAGE_COMMAND_ERROR: _("Unable to extract archive: %(error)s"),
+    }


Another case where the formatting is getting a bit messy.

These lines are being selectively wrapped due to the line length limit.

I've wrapped the block with # fmt: off and # fmt: on to skip it; I'm not sure of a a better approach right now.

This was a minor issue anyways. Good to know about the # fmt blocks. This might be useful in the future as well when we want to force a particular layout for readability reasons.

rdmark · 2022-11-30T02:32:10Z

python/web/src/web.py

-        help="Run in development mode"
-        )
+    )
+    parser.add_argument("--dev-mode", action="store_true", help="Run in development mode")


I wish the formatter hadn't removed the indentation here either.

Sorted with trailing comma.

nucleogenic · 2022-11-30T03:51:54Z

@rdmark Thanks for the comments. In short, I agree! I think we can disable the auto-formatting in some of these examples where it's particularly egregious. I'll see what that looks like and push an update.

rdmark · 2022-11-30T05:15:13Z

@nucleogenic Please go ahead with merging this at any time. I don't think further code review is required.

nucleogenic added 2 commits November 29, 2022 22:56

Update config for black and flake8

a1ff49d

Auto-format Python sources with black

9fb6430

WIP WIP WIP

nucleogenic requested review from akuker, erichelgeson and rdmark as code owners November 29, 2022 23:06

nucleogenic added 2 commits November 29, 2022 23:06

Fix issues reported by flake8

86e4564

Exclude protobuf files from black

feccfb4

nucleogenic force-pushed the nucleogenic-python-black-autoformat branch from 3cf8844 to feccfb4 Compare November 29, 2022 23:07

rdmark approved these changes Nov 30, 2022

View reviewed changes

Address formatting feedback

d8be315

nucleogenic force-pushed the nucleogenic-python-black-autoformat branch from 1c71b00 to d8be315 Compare November 30, 2022 05:02

nucleogenic requested a review from rdmark November 30, 2022 05:03

nucleogenic mentioned this pull request Nov 30, 2022

Run web API test suite in GitHub Actions #1009

Merged

rdmark approved these changes Nov 30, 2022

View reviewed changes

Add TODO

f5e7d38

nucleogenic merged commit 315ef9f into develop Nov 30, 2022

nucleogenic deleted the nucleogenic-python-black-autoformat branch November 30, 2022 05:19

Uh oh!

Conversation

nucleogenic commented Nov 29, 2022

Uh oh!

rdmark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nucleogenic commented Nov 30, 2022

Uh oh!

rdmark commented Nov 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants