Migrate codebase to ruff 0.16+ - #361
Merged
Merged
Conversation
This is an obnoxious change. I could have just disabled the new rules and maintained the old behavior but I figured that if I don't tackle this now, I never will. This is the first pass of easy fixes, more complex ones will come in follow-up commits, so I can keep track of my own bullshit better. * Condensed with statements * properly type classvars and similar, * Fix exception string formatting (remove str casts) * catch OSError instead of old school IOError which I only just learned today has been deprecated since forever. Awkward. * Narrowed broad exception catches in a handful of spots where they are clear and straightforward * Narrowed textual QueryError catches to specific subtypes, since, well, they are known. It's not like there'd be a surprise, so yes, might as well. This happening at runtime is a bug in any case. * Justified and noqa'd broad exception catches where they are absolutely warranted (and that's many of them, honestly). * Removed the UTCDateTime TypeAlias which was some cruft left from the transition away from naive datetimes -- which the linter now actually warns about anyways. It served no purpose at best, and was misleading at worst, so I just replaced it with an Annotated type hint. * Replace UTC datetime setup with Python 3.11's datetime.UTC, which is a neat spoonful of syntactic sugar that is arguably nicer. * Added a proper logger instance to the textual app root module, instead of just piping into the root logger -- this was originally done for logger setup reasons, but the unit tests around this are going to get cleaned up in a follow-up commit, so we fix this now. Fixing this highlighted a minor design flaw in how we handle Fabric and Paramiko exceptions, which I have a minor refactor in the works to address, also in another commit. Some exceptions still need narrowed as of this commit, but they need looked into more closely, and the Config system will probably just normalize most of its loader related issues into ValueError for simplicity since programmer errors with bad loaders aren't really a user facing problem.
Since the loader portion of a configuration file can be arbitrary, the API now standardizes on ValueError as what is raised whenever a configuration file does not parse, or a loader callable raised anything that wasn't OSError. What any particular loader might raise when they fail to parse data is unknowable, so this gives a nice, predictable surface, with no information loss.
With one exception that actually tests the actual logger setup mechanics, and is generally safe, the other UI logging tests have all been rewritten to just use the caplog fixture. There is always inherent state leak risks between logs when patching logging directly, which the fixture does better anyways.
Since we run everything through poe and uv it hardly matters, and those aren't intended to run outside the venv.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ruff 0.16 enables a significant of new linting rules by default, which are, in a vacuum, good to have.
I could have just configured it to keep what 0.15 and below had and moved on with my life, but unfortunately, I ended up caring, and this led to some minor refactors.
The opportunity was taken to also clean up the the test suite a little, at least with all the dirty mocking of the logging module I was doing out of inertia from having to test the various subsystems around it for the TUI Log Screen. It's all been replaced with standard caplog whenever possible.
Internally, the configuration load errors have all been normalized to ValueError since defining the exact scope of arbitrary loaders is not exactly possible, and the user does not care about the distinction, ultimately.
Additionally, a handful of libraries and dev tools have been bumped in uv.lock because the opportunity to test was present.