-
Notifications
You must be signed in to change notification settings - Fork 0
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
Delete the LandMine
class
#6
Conversation
I don't think this is correct. The LandMine class is needed for custom converters. We don't have a lot of coverage for those; we should fix that. I was able to trigger the LandMine class last year when experimenting with custom converters for sqlite3, IIRC. I'm sure we can fix it with a special case of Function, though; it does not have to be a separate class. And I think we should rename it to something nicer; land mines are not nice. |
Hmmm, I'm still not quite sure I fully understand the situation where setting
Yes, maybe it can be a subclass of
indeed |
I have a test ready. Let's add that first :) You may be right; it could be sufficient to set |
See python#107496 |
That wouldn't improve anything from a typing perspective: What I'm suggesting is just not having We could possibly add a custom |
Aha, I misunderstood you. Yes, I like that. |
Thoughts on the updated version? |
It's nice. I think it would be helpful to include the attr name in the error message, since that can help debugging. |
Hmm. Now the attribute error is raised trying to access |
Hm, true. Calling (BTW; the line numbers in the tests messed up because of FakeClinic; that's why most of the tests report the line number as 0.) |
Let's take this to the cpython repo :) |
By the way, one of the comments mention that the LandMine class is there to protect against certain types of cloning. We should try and add a test for that case as well; it may take a slightly different path, so we need to make sure we catch that (potentially) slightly different scenario. |
Re-reading that comment, it kinda looks like we're covered. BTW, here's the commit that introduced it: 7726ac9 |
whelp, there's a lot going on there |
IIRC, they used to queue approved commits, and them squash them all with a giant change log entry and/or git commit message. There's a lot of commits like that from the way old development workflow :) |
I filed python#107541 |
@erlend-aasland, what do you think about this? It simplifies some of the typing, since now
CConverter.function
is always an instance of Function, instead of sometimes being an instance ofLandMine
. I'm not sure why we need to temporarily setself.function
to be an instance ofLandMine
-- I feel like we can get the same effect by just not settingself.function
until afterconverter_init()
has been called. But I'm a bit wary of Chesterton's Fence here -- I'm not sure I understand fully exactly which problem theself.function = LandMine()
thing was originally supposed to solve.