-
Notifications
You must be signed in to change notification settings - Fork 58
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
Record: force name to be given by name for __init__()
#266
Conversation
Codecov Report
@@ Coverage Diff @@
## master #266 +/- ##
=======================================
Coverage 82.07% 82.07%
=======================================
Files 34 34
Lines 5597 5597
Branches 1200 1200
=======================================
Hits 4594 4594
Misses 863 863
Partials 140 140
Continue to review full report at Codecov.
|
I think the change is good (everywhere else |
This is to make it easier to support the `like()` class method in subclasses of Record. If in the subclass the second argument to `__init__()` is not name one currently has to take into account that the second argument can actually be the name if the first argument is of type `Layout`. For example if you have: class JTAG(Record): def __init__(master=False, trst=False, *, name=None, src_loc_at=0): If one uses the .like() function on a JTAG object it will pass name in trst and not in name without this patch.
Updated comment, added: For example if you have: class JTAG(Record):
def __init__(master=False, trst=False, *, name=None, src_loc_at=0): If one uses the .like() function on a JTAG object it will pass name in trst |
After some reflection I think some more documentation or changes need to be done to handle calling .like() for subclasses of Record. jtag = JTAG()
jtag_like = Record.like(jtag) jtag_like is of type Record and not JTAG. Is this wanted behavior ? |
I've merged the PR but with a completely different rationale, see the updated commit message.
Yes. There is no general way to handle this such that |
I do think there is a way to do it if you force that subclasses make their class Record(Value):
@classmethod
def like(cls, other, *, name=None, name_suffix=None, src_loc_at=0):
assert isinstance(other, cls)
if cls != other.__class__:
return other.__class__.like(other, name=name, name_suffix=name_suffix, src_loc_at=src_loc_at) Reason I don't like that calling |
The branch in your example will never fire, unless I'm misreading it.
You should in general not overload
You can override class methods just fine. |
Maybe we can solve the |
No, if you call
and then use |
Ah, right. |
But maybe nothing has to be changed, just a comment in the code class Record(Value):
@classmethod
def like(cls, other, *, name=None, name_suffix=None, src_loc_at=0):
"""Return fresh object with same fields as other object
Use Record.like(other) if one wants a Record object with same fields as other.
Use other.__class__.like(other) if one wants an object of same class as other
and with same fields.""" |
I like that. I'll make sure to mention it when I add some docs for |
This is to make it easier to support the
like()
class method in subclasses of Record. If in the subclass the second argument to__init__()
is not name one currently has to take into account that the second argument can actually be the name if the first argument is of typeLayout
.