Code cleanups based on pylint linting
#67
Conversation
The word `license` is generally a reserved word because unless explicitly told not to with the `-S` option Python will import the `site` module which contains this word. Using it in variable names etc. redefines this imported name which is undesirable behavior.
You should not use a function's name as a variable name within the same function.
The `logging.warn()` method was deprecated in Python 3.3 in favor of the `logging.warning()` method.
Fix exception message to provide a formatted string. Use raise...from syntax to provide additional information when raising an exception after catching another exception.
Open files with a context manager to ensure that they are closed once they are no longer needed.
The print() function always returns None which is also the default function return value. This allows us to remove the return statement and the else block.
There is no need to have alternative logic (elif/else) blocks after an if statement that has a return or raise statement.
Instead of using if not greater than it is clearer to use if less than or equal.
Since we are checking the same variable for different possible values it is cleaner to use the `in` statement and a tuple of possible values.
The get_all_projects() function returns a list and since we are not modifying the values there is no need to create a new list to return.
Lazy logging is what is recommended by the logging library and is what is used elsewhere in the project for logging messages.
Exception constructors do not perform string interpolation when given multiple arguments so strings must be interpolated before being passed as arguments.
It is best practice to specify the encoding to use when reading and/or writing to files. This helps ensure consistent operation across different OS types/versions compared to using whatever the system default is for that particular system.
|
|
||
| project["status"] = status | ||
| project["status"] = "Production" if status else "Development" |
There was a problem hiding this comment.
Is this one of the pylint recommendations?
I actually was discussing this difference in style earlier this week with a colleague.
There was a problem hiding this comment.
So pylint's flag and remediation recommendation is: Unnecessary "elif" after "raise", remove the leading "el" from "elif" (no-else-raise). The change you marked was just my personal style coming out and I'm happy to explicitly follow the pylint remediation if you prefer. I just tend to use the x if condition else y flow for simple assignments as I find it cleaner.
| @@ -73,7 +77,7 @@ def _num_requests_needed(num_repos, factor=2, wiggle_room=100): | |||
| return num_repos * factor + wiggle_room | |||
|
|
|||
|
|
|||
| def _check_api_limits(gh_session, api_required=250, sleep_time=15): | |||
There was a problem hiding this comment.
Rather than removing it here, should the time.sleep(10) line at the end of this function be updated to use this sleep_time argument?
There was a problem hiding this comment.
So I was on the fence about which way to go on that decision. When I looked into the blame I found that even when this function was first added the argument was never used. Looking at the code I feel like using the value of time_to_reset makes the most sense as we already calculate a specific time until the API limit is reset. I was trying to make no changes to functionality with this PR so I just went with removing the unused argument in this case.
Yes we (over at @cisagov) lean heavily on pre-commit for code quality checking and our configuration includes the same tools you use here. I've been looking at integrating |
This pull request is an assortment of small changes based on running
pylintwith a default configuration with the following (modified from default) list of disabled checks:disable=raw-checker-failed, bad-inline-option, locally-disabled, file-ignored, suppressed-message, useless-suppression, deprecated-pragma, use-symbolic-message-instead, too-many-arguments, too-many-locals, too-many-return-statements, line-too-long, missing-module-docstring, missing-class-docstring, missing-function-docstring, invalid-name, too-few-public-methods, too-many-branches, too-many-instance-attributes, too-many-statements, fixme, consider-using-f-string, super-init-not-called, protected-accessAll other items flagged were fixed. I also noticed an issue with how the messages for
raisecalls inscraper/tfs/__init__.pywere formatted and fixes those while I was making these changes.