-
Notifications
You must be signed in to change notification settings - Fork 0
Module and class resolver #24
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
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, this is a big readability improvement!
module = parent.modules[field] | ||
parent = module |
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.
I actually find putting both assignments on one line more readable -- conceptually, you're assigning both variables to the same thing here (parent.modules[field]
), rather than assigning module
to parent.modules[field]
, and then assigning module
to whatever parent
is:
module = parent.modules[field] | |
parent = module | |
parent = module = parent.modules[field] |
cls = parent.classes[field] | ||
parent = cls |
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.
Same here, I actually find it more readable to have them both on one line:
cls = parent.classes[field] | |
parent = cls | |
parent = cls = parent.classes[field] |
cls = parent.classes[field] | ||
parent = cls | ||
else: | ||
fullname = ".".join(fields[idx]) |
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.
I. .t.h.i.n.k. .t.h.i.s. .i.s. .g.o.i.n.g. .t.o. .p.r.o.d.u.c.e. .o.u.t.p.u.t. .l.i.k.e. .t.h.i.s
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.
>>> fields = ["foo", "bar"]
>>> idx = 1
>>> ".".join(fields[idx])
'b.a.r'
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.
😂 There was supposed to be a : somewhere there
Shall we upstream this? 😃 |
I vote yes! We might want to add a regression test for https://github.com/Argument-Clinic/cpython/pull/24/files#r1298346341, though :) |
Absolutely; I'll do that first :) |
Upstreamed at python#108552 |
Experimenting with variants of this resolver.