Refactoring: fix.py / linting code files / added Types #48
Conversation
|
Thanks, this is great, and I'm open to merging this. Though, do you mind breaking this down into smaller commits? It'll help with debugging/bisecting in case there's a potential bug from the refactor. |
|
Yeah, sorry about that. I intended to do smaller commits, but somehow I ended up doing too much at once and it was hard to break it up into separate commits afterwards. Most changes are trivial, as types are not enforced by Python. I will try to split it up, that might take some time. |
| from .logger import log | ||
|
|
||
|
|
||
| def esync_file_limits(): | ||
| def esync_file_limits() -> bool: |
There was a problem hiding this comment.
Hmm if we're adding types to non-gamefixes, type annotations should probably be enforced by the linter, Pylint, and caught in the CI system to avoid having to add the missing type in the future. This can be addressed in a separate PR though
There was a problem hiding this comment.
We could also enforce types for the gamefixes.
This should be trivial, as most of them only consist of the main() function.
There was a problem hiding this comment.
We could and it'll be keep things consistent. Though, I don't think it'll add much though so you don't need to.
For the game fixes, what we really want to catch from the linter are the typos, logical errors and some bad practices. There were just a ton of them before I added the linter in this project, and for the subset of fixes that return a non-None type for some reason I don't think it'll affect the behavior of the program.
I won't stop you from doing it though.
There was a problem hiding this comment.
Sure, I think it is just easier to maintain one linter configuration than multiple. Alternatively there might be an option to allow functions that return None to have no type. Not sure about that though.
| """ Retrieve a filename from a request headers via Content-Disposition | ||
| """ | ||
| content_disp = [x for x in headers if x[0] == 'Content-Disposition'][0][1] | ||
| raw_filename = [x for x in content_disp.split(';') if x.startswith('filename=')][0] | ||
| return raw_filename.replace('filename=', '').replace('"', '') | ||
|
|
||
|
|
||
| def gdrive_download(gdrive_id, path): | ||
| def gdrive_download(gdrive_id: str, path: str) -> None: |
There was a problem hiding this comment.
This functionality was inherited from upstream and it's not being used anywhere in this project.
Personally, I haven't verified if it actually works either, and if possible, I'd like to remove this functionality all together. But since users might be using it in their local fixes, I guess we can't really do that.
There was a problem hiding this comment.
I have come across a number of functions that I am convinced are not being used. I left them in and updated them anyway, as it wasn't in the scope of this PR.
Some stuff could be broken up, like certain functions of the utils.py.. but then again some users may need to update their local fixes.
There was a problem hiding this comment.
For this specifically, if it can be proved that the google drive functionality does not work, then that would warrant removing it. Of course, it doesn't have to be addressed in this PR.
In a future separate PR, we can definitely evaluate removing some of them too. Or like, wait until the upcoming new year for the cleaning 😄
| module_name = game_id if not default else 'default' | ||
|
|
||
| # Check if local gamefix exists | ||
| if not os.path.isfile(os.path.join(localpath, module_name + '.py')): |
There was a problem hiding this comment.
There are more of these, I updated strings that are formatted or consist of multiple concatenations.
I left strings that are simple prefixes or suffixes.
I could change them too, and we could also enforce them through the linter.
|
|
||
|
|
||
| def sha1sum(filename): | ||
| def sha1sum(filename: str) -> str: |
There was a problem hiding this comment.
Same with this function. It's sorta useless and it makes more sense if the winetricks executable uses it. But unfortunately it's a shell script, and for the same above reasons, we'll probably just have to leave it
Minor refactoring of engine.py
- Cleaned imports - Some string formats - Prefere single quotes - Whitespaces - Valid variable names - Provide module description for all modules - Misc util.py: - Unified write and create of disable_uplay_overlay() - Simplyfied / improved set_xml_options() - Use exception types in try_show_gui_error() steamhelper.py: - Prevent usage of global keyword - STEAM_DIRS is constant, therefore global fix.py: - Leaner use of check internet() - Return title without loading file - Fixed order of Exceptions (OSError is an ancestor class of TimeoutError) - Renamed gameid -> game_id - Renamed game_id() -> get_game_id() - Renamed game_name() -> get_game_name() - Unified exception handling, the linter counts them as statements - Make use of logger's debug function for exception handling - Fixed bad indentation
- Make use of "with" keyword - Specified encoding for files - Specified timeouts for "urlopen" fix.py: - Unified request for store and special case 'none'
- Refactored run_fix() to just call internal _run_fix() and _run_fix_local() multiple times, much leaner - Implemented internal _run_fix_local(), which wraps _run_fix() to search for local fixes and ensure they are importable - Implemented internal _run_fix(), which imports and runs gamefixes - Implemented get_module_name(), which generates a string representing the module's name to import - Added get_store_name(), a mapping of 'STORE' env var to a canonical name for each store - Most logging is now dynamic, which deduplicates quite some code - Added caching to get_game_id() and get_game_name(), as they might spawn heavy IO / web requests if called multiple times
- Added better descriptions to exceptions - Removed exception for C0114, as it was fixed by adding docstrings to all modules
Refactor:
Linter: