-
-
Notifications
You must be signed in to change notification settings - Fork 795
✅ Use Ruff rules to ensure safe lazy-loading of rich
#1297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✅ Use Ruff rules to ensure safe lazy-loading of rich
#1297
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for working on this! I'm putting this in draft as long as the CI fails - feel free to mark as ready for review when the CI goed green! |
rich lazy-loading safelyrich
This comment was marked as outdated.
This comment was marked as outdated.
86aeb12 to
4e75183
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@svlandeg Alright, I think it's good now. I can't add labels so that check's failing, but everything else is green. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
669a873 to
562b36f
Compare
…t_tracebacks.py::test_traceback_no_rich` so that it monkeypatches the correct module, i.e. `typer.main` rather than `typer.core`
…rich_short_disable.py`
562b36f to
4bbe297
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, so a recap.
This PR does a number of related things:
- Avoid importing
rich_utilsat the top of any module, ensuring its lazy loading from #1128 (flake8-tidy-imports) - Avoid importing
richanywhere in the codebase, except inrich_utils.pyand docs files (flake8-tidy-imports.banned-api) - Refactor how to define and store
HAS_RICH: usingimportlib.util.find_spec("rich")instead of atry-catchimport incore.py - Fetching the
HAS_RICHvar fromcore.pyin bothmain.pyandcli.pyto avoid code duplication and have easier maintenance in the future - Refactor the tests to use
HAS_RICHandpytest.MonkeyPatchinstead of setting thetyper.core.richvariable directly.
To me, 1 & 2 are the crucial points of this PR: they ensure we're not importing rich or rich_utils by accident. This would have caught #1290 as well as a similar issue we encountered in #828: it will be nice to set things up so that we can't hit ourselves with this again 🥲
3 follows out of this because you can't import rich anymore, and in fact having a HAS_RICH variable feels like a nicer solution as well, as compared to our old solution of rich either being an imported library, or None.
4 was added to this PR by my suggestion as I think it's more clean to define this functionality only once and then import.
5, the refactoring of the tests, needs to happen anyway when switching from the rich variable check for None to the boolean HAS_RICH. Using MonkeyPatch to do this feels more elegant to me than what we had before.
So, all in all, this looks (really) good to me. Thanks again @nathanjmcdougall for all your work on this!
I'm passing this on internally to Tiangolo's reviewing queue, he will update here once he's had a chance to review 🙏
This comment was marked as outdated.
This comment was marked as outdated.
|
After this PR is done, I plan do the same but with |
|
📝 Docs preview for commit d8ffd89 at: https://07df5e78.typertiangolo.pages.dev |
|
Awesome, looks great! Thanks for all the work on this @nathanjmcdougall! 🙌 And thanks @svlandeg for the thorough explanation of everything and the reasons for it all. 🚀 |
To avoid regressions like #1294 occurring in the future, I propose a different way of lazy-loading rich using
importlib.util.find_spec. I also propose using theTIDRuff rules to enforce the lazy-loading logic in the future.This provides a framework to systematically apply other lazy-loading migrations safely.