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
Group compatibility imports into a single fixer #295
Group compatibility imports into a single fixer #295
Conversation
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.
Thanks for your patience... my son was born the week of your PR, so I've been somewhat busy since! I think this is a good change, will really reduce toil in adding such replacements, and nice little speed boost. I am changing some of the variable names to make it more obvious, and adding a changelog note, but other than that, no comments. Nice one again @UnknownPlatypus .
fc864bc
to
7003b8f
Compare
@lru_cache(maxsize=None) | ||
def _get_replacements( | ||
version: tuple[int, int], looks_like_migrations_file: bool | ||
) -> Mapping[str, dict[str, str]]: |
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.
one tweak here: making the return type a Mapping
allows Mypy to ensure the dict doesn't get mutated outside this function
for old_module, rewrite in target_replacements.items(): | ||
replacements[old_module].update(rewrite) | ||
|
||
replacements.default_factory = None |
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.
second tweak: setting default_factory
to None
makes this act like a regular dict after this point. always a good idea with defaultdict
.
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.
oh nice one! Didn't thought about that.
oh absolutely no worries! Thanks for the few tweaks and for the changelog, I wasn't sure if you wanted to mention it since it's mostly internal changes. |
Thank you, yes it is!
I think the performance improvement is worth mentioning :) See you at Djangocon Europe this year? |
Would love to participate to one of these but I'm not sure I'll be available and have the time/budget to travel there. Will you be doing some talks this year ? PS: Unrelated but what is your preferred way of contact for a suggestion on one of your blog post ? email ? twitter? |
I've submitted to give a talk, hopefully I get selected. I'll be there regardless.
Email preferred but anything works: https://adamj.eu/contact/#details |
Hi !
Here is the grouping part discussed in #293.
I measured a ~3% performance improvement on the same test repository I generally use (5k python files, 483kloc).
I think it's really cheap now to add the older compatibility imports given the new structure.
Let me know how you feel about it.