-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Adds support for fields with local dataclass types #180
Conversation
Hi @mishamsk Thank you for bringing attention to this problem. I've been placing dataclasses globally in the tests, trying to postpone the inevitable :) In fact, I have already made preparation to solve this problem with local types. There is mashumaro/mashumaro/core/meta/types/common.py Lines 306 to 309 in 517d8d5
It would be safer to use it because someone might have more than one local type. I've made a quick proof of concept in this commit: mashumaro/mashumaro/core/meta/code/builder.py Lines 563 to 565 in 5e0b05b
But the most important thing is that |
11408e3
to
ba0483e
Compare
@Fatal1ty well, I wasn't sure how far you'd want to go with refactoring the code. First using the existing helpers obviously makes sense - I added a commit to PR, which uses what you've suggested except for two changes:
I get the fact that this won't solve the issue completely, but it doesn't mean it can't be merged (I guess). I reviewed It feels a bit daunting to add a fork in every single place where It is a pretty big change either way. Maybe worth doing it gradually (e.g. for Mixin subclasses only first, then codecs). What do you think? |
Yeah, it looks better this way!
Absolutely. One step at a time is better than nothing.
This would be a big change that is likely to affect performance. Some time ago I was tempted by eliminating qualnames, thinking that one name instead of "getattr chain" would speed things up, but to my surprise performance was getting the same or even worse. However, I admit that something might have changed since then and it's worth trying again.
I totally agree. I will review your changes soon. |
I haven’t made changes across the codebase, wanted your thoughts on it first. So we are narrowing the scope to mixins only.
Interesting. I was expecting python to create a closure when it runs I guess I can start adding bindings for local objects anyways. To switch everything to locals or not may be a later stage decision… |
add more types to test
bf8ec15
to
4998a14
Compare
so this fixes #182 what I did since the last commit:
I reverted and renamed the Alternative would be to change type_name return to also produce a flag for local types, but that would yield so many changes in the code, I thought it is not worth it. I'd vote to merge it this way, as it should cover 99% of real-life cases...but up to you. I am eagerly waiting for this;-) |
Sorry for late answer, the New Year's Eve fuss takes up a lot of my time.
You made huge work here but I have a small suggestion about local type names. It seems like it would be practical if we encapsulate checking for the localness and adding to the "globals" in a new
Great, we're on the same page here. I also think that there is no need to change it there. P.S. The next time I will be available in early January. Merry Christmas and a happy New Year, take more time off from the computer! 🎅 |
not surprised;-)
not a big fan of methods lately, but sure, that does make sense. applied the change. except that I named it
Thanks, same to you!
sounds like my wife told you something;-) |
I added usage of Thank you for this pull request. You did it exactly the way I would have done it. |
Discriminators are fixed in #189. |
Hey,
This PR makes the following work:
First why: as you know, I use a custom deserialization base class that maintains a global registry of DataClassDictMixin subclasses allowing discrimination by subclass. I haven't migrated to 3.10, so it is still there.
This in turn means that at any given moment all subclass names must be unique. This is inconvenient for tests, but since so far mashumaro enforces classes in global scope, there is no easy way to create isolated tests, that define classes and then remove them.
Not sure if there are a lot of real life use cases outside of tests, but there may be...
The way I've implemented it is minimally disruptive. It should change nothing unless the type annotation is a locally defined class. Generally speaking, the current code generation can stop relying on fully qualified names of types entirely and use my change that just adds them to locals before executing. But that may have bigger consequences, so I opted no to do that.
Fixes #182