Static type checking / problem annotation / improvements / refactoring #152
Conversation
Root-Core
force-pushed
the
improvements
branch
from
0f58f93 to
09fa421
Compare
Root-Core
force-pushed
the
improvements
branch
2 times, most recently
from
eeecbd5 to
d414934
Compare
This prevents subsequent errors and fixes errors thrown by static code analysis.
- Correctly override optionxform - String can not be None - Fixed import of urllib - Exit if game id is not present - Allow paths to be str or Path
- Typed config - Separate sections in object - Easy and clean interface - Reusable
- use print functions instead of manual - class methods - caching - format string
Root-Core
force-pushed
the
improvements
branch
from
d414934 to
739a137
Compare
- Changed ruff's docstring style to "google" - Removed ignored ruff rules, that are already satisfied - Ignore missing cls annotations (ANN102) - Added docstrings to Config and ConfigBase (+Engine)
- enable static type checking via pyright-action - added pyproject.toml with basic meta data and pyright configuration - use action-shellcheck instead of installing it manually - use ruff-action instead of installing it manually - Cache pip dependencies
- Use pathlib if possible - Return Path() object is possible - Parse ProtonVersion correctly - Reimplemented get_resolution() with RegEx - Reimplemented _get_case_insensitive_name() - Reimplemented _killhanging() - Reimplemented _checkinstalled() - Improved some logging - Some caching
util.py: - Added create_dos_device() - Added patch_conf_value() - Added patch_voodoo_conf() - Added get_path_syswow64() - Moved class ReplaceType from Bethesda mod support (class Redirect) Other: - Refactored gamefixes to use pathlib - Simplified / reimplemented some fixes - Some docstrings - Linked Gobliiins 5 demo to main game - Unified Gothic 3 and Gothic 3 Forsaken Gods Enhanced Edition - Fixes after rebase
- Logger: use Enum values for colors - Use default parameters, if argument is equal
- Fixed relative imports - Removed import handling via ImportError exception - Fixed CI by running via module, instead of file - Added / improved handling in __init__.py for unit tests
Root-Core
force-pushed
the
improvements
branch
from
739a137 to
71eb0da
Compare
|
The PR should be ready to go. I know it's heavy... but if you go through it commit by commit, it shouldn't be too complex. Sorry about that. Splitting wasn't really an option. If it gets merged, I would vote for keeping the commit history and avoiding squashing it. I finally tested it and fixed the last remaining issue, the "ugly" imports for the unit tests. Disclaimer: I have only tested it with Steam and it works for me. It would be nice if someone could test it on other platforms. |
I'm open to merging some of this, but are you sure splitting it wasn't really an option? For instance, at a glance, I'm not seeing why the config-related functionality couldn't be. |
|
We're making a lot of changes here on functionality that isn't currently unit tested. More importantly, it affects user created fixes that are not being tested in the CI either, which gives me less confidence. Unless you think that only using ruff is sufficient? |
|
In our CI, I'm thinking it might be a good idea to simply run all our current fixes through umu-launcher. It won't guarantee that the fix works for the game, but it will guarantee a runtime error won't occur whenever we do big refactors. |
| @@ -57,20 +57,8 @@ select = [ | |||
| "D" | |||
There was a problem hiding this comment.
We can enforce using pathlib through the PTH rule whenever working with paths.
See https://docs.astral.sh/ruff/rules/#flake8-use-pathlib-pth
|
To be honest, I don't remember what the problem was with the config. I think there were some issues with the static types and it still needed to be rewritten. It might be a bit overkill with the current setup, but I think it's nicely typed and clean this way. Sure, some of the refactoring could be excluded, mostly the refactored gamefixes, but again this is heavily tied to the transition to pathlib and fixing some of the typing issues. The reason why splitting it up is a bit complicated is that the static type checks are active once the workflow is merged, and when it is merged, it will generate error messages and annotations for every PR until all issues are fixed. So undoing things will certainly break things, and it will create quite a bit of work to work around it. You might disagree, but if we end up merging all/most of it, it wouldn't be worth the effort to artificially split the PR just for the sake of it. I know this isn't satisfying, but please just check commit for commit. Most of it is noise, and the rest is actually trying to improve and fix things. |
This PR enables static type checking and annotation of CI issues on GitHub.
It has not been tested much though, so I am opening it as a draft PR.
I'm also not sure if the relative imports will cause problems.
Feedback and testing welcome.
pathlibinutil.pyand gamefixesutil.pyutil.pyConfigBaseclasslogger.pyCC #50