Maint/fix manual #3
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving code readability and maintainability by removing unused variables, fixing typos, and enhancing docstrings across multiple files. The changes are primarily aimed at cleaning up the codebase without altering the core functionality. File-Level Changes
Tips
|
There was a problem hiding this comment.
Hey @Remi-Gau - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 10 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -40,11 +40,11 @@ def __init__( | |||
| ): | |||
| try: | |||
| self.stdout = stdout.decode("utf=8", "backslashreplace") | |||
| except AttributeError as e: | |||
| except AttributeError: | |||
There was a problem hiding this comment.
suggestion: Remove unused exception variable.
The exception variable 'e' was removed from the except block. This is a good practice if the variable is not used, but ensure that no logging or debugging information is lost.
| except AttributeError: | |
| except AttributeError: | |
| logging.debug("AttributeError encountered, setting self.stdout to stdout") |
| @@ -246,7 +248,7 @@ def execute(self, mount_strings): | |||
| conType, conImage = con.get("type"), ( | |||
| con.get("image") if not self.noContainer else None | |||
| ) | |||
| conIndex = con.get("index") | |||
| _ = con.get("index") | |||
There was a problem hiding this comment.
suggestion: Consider removing unused variable assignment.
The variable 'conIndex' was replaced with '_', indicating it is unused. If this value is truly unnecessary, consider removing the assignment entirely to clean up the code.
| _ = con.get("index") | |
| conOpts = con.get("container-opts") | |
| conIsPresent = conImage is not None |
| @@ -1070,7 +1072,7 @@ def checkMutualRequirements(targ): | |||
| for r in result: | |||
| self.in_dict[r["id"]] = makeParam(r) | |||
| # Check for mutex between in_dict and last in param | |||
| for group, mbs in [ | |||
| for _, mbs in [ | |||
There was a problem hiding this comment.
suggestion: Consider removing unused loop variable.
The loop variable 'group' was replaced with '_', indicating it is unused. If the variable is truly unnecessary, consider refactoring the loop to avoid creating unused variables.
| for _, mbs in [ | |
| for mbs in (x["members"] for x in self.groups): |
| @@ -430,7 +430,7 @@ def parse_req(req, req_type, bout_desc): | |||
| if req.get("ramMin"): | |||
| suggested_resources["ram"] = req["ramMin"] | |||
| if req.get("coresMin"): | |||
| suggeseted_resources["cpu-cores"] = req["coresMin"] | |||
| suggested_resources["cpu-cores"] = req["coresMin"] | |||
There was a problem hiding this comment.
issue (typo): Fix typo in variable name.
The typo 'suggeseted_resources' was corrected to 'suggested_resources'. Ensure that this change is reflected throughout the codebase.
| @@ -83,7 +83,7 @@ def execute(*params): | |||
| try: | |||
| # Try to parse input with argparse | |||
| results, _ = parser.parse_known_args(params) | |||
| except SystemExit as e: | |||
| except SystemExit: | |||
There was a problem hiding this comment.
suggestion: Remove unused exception variable.
The exception variable 'e' was removed from the except block. This is a good practice if the variable is not used, but ensure that no logging or debugging information is lost.
| except SystemExit: | |
| except SystemExit as e: | |
| print_info(execute.__doc__) | |
| raise_error(ExecutorError, f"Incorrect usage of 'bosh exec': {e}") |
| @@ -188,7 +181,7 @@ def getSubstringType(s): | |||
| for mid in inIds | |||
| if inById(mid)["value-key"] == key | |||
| ] | |||
| for idx, grp in enumerate(descriptor.get("groups")): | |||
| for _, grp in enumerate(descriptor.get("groups")): | |||
There was a problem hiding this comment.
suggestion: Consider removing unused loop variable.
The loop variable 'idx' was replaced with '_', indicating it is unused. If the variable is truly unnecessary, consider refactoring the loop to avoid creating unused variables.
| for _, grp in enumerate(descriptor.get("groups")): | |
| for grp in descriptor.get("groups", []): |
| @@ -205,7 +203,7 @@ def parseAction(self, action, **kwargs): | |||
| actstring = str(type(action)) | |||
| actstring = actstring.split("'")[1].split(".")[-1] | |||
| print_info(f"{actstring}: Adding") | |||
| actdict = vars(action) | |||
| _ = vars(action) | |||
There was a problem hiding this comment.
suggestion: Consider removing unused variable assignment.
The variable 'actdict' was replaced with '_', indicating it is unused. If this value is truly unnecessary, consider removing the assignment entirely to clean up the code.
| _ = vars(action) | |
| # Removed unused variable assignment | |
| if action.dest == "==SUPPRESS==": | |
| adest = f"subparser_{self.sp_count}" |
|
|
||
|
|
||
| def compute_md5(filename): | ||
| with open(filename, "rb") as fhandle: | ||
| with open(filename, "rb"): |
There was a problem hiding this comment.
issue (bug_risk): Fix file handling issue.
The file handle 'fhandle' was removed, but the file is still being opened. Ensure that the file is properly handled and closed to avoid resource leaks.
| for r in results: | ||
| for k, v in r.items(): | ||
| for _, v in r.items(): | ||
| self.assertLessEqual(len(str(v)), 43) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for _, v in r.items(): | ||
| self.assertLessEqual(len(str(v)), 43) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Pull Request Test Coverage Report for Build 10041893254Details
💛 - Coveralls |
name: Feature or bug fix name
about: Propose modifications in the code
title: 'Feature implementation | Bug fix for issue #<000>'
labels: enhancement
assignees: ''
Related issues
Checklist
PEP 257).
Purpose
Current behaviour
New behaviour
Does this introduce a major change?
Implementation Detail
Summary by Sourcery
This pull request includes bug fixes for exception handling and a typo correction. It also enhances the codebase by improving docstrings, replacing string formatting with f-strings, and removing unused variables.