Codecov Report

Merging #266 into master will not change coverage.
The diff coverage is 100%.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9749c70...5f5c07d. Read the comment docs.

I think the change is good (everywhere else name is keyword-only already), but I have no idea what your PR description is supposed to mean.

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
and not in name without this patch.

After some reflection I think some more documentation or changes need to be done to handle calling .like() for subclasses of Record.
For the same JTAG class, if one does:

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.

jtag_like is of type Record and not JTAG. Is this wanted behavior ?

Yes. There is no general way to handle this such that like returns an instance of the subclass. The generic Record.like should be changed to use Record(...) instead of cls(...) and any subclasses that want like to return a subclass instance should reimplement it themselves.

Yes. There is no general way to handle this such that like returns an instance of the subclass. The generic Record.like should be changed to use Record(...) instead of cls(...) and any subclasses that want like to return a subclass instance should reimplement it themselves.

I do think there is a way to do it if you force that subclasses make their .like() parameters compatible with the parent:

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 Record.like(jtag) results in a Record object is interaction with overloaded methods. If JTAG overloads Record methods like for example connect() that overloaded behavior will not be seen on the object coming from Record.like().
The question is actually what the use case is for Record.like() and what can and can't be done with the result of it.
Should .like() actually need to be a class method ? Can't it just be a normal method that then can be overloaded by the JTAG class ?

I do think there is a way to do it if you force that subclasses make their .like() parameters compatible with the parent:

The branch in your example will never fire, unless I'm misreading it.

If JTAG overloads Record methods like for example connect() that overloaded behavior will not be seen on the object coming from Record.like().

You should in general not overload connect(), definitely not if you violate LSP when doing it. In general, the use of Record.connect is often ill-advised and almost always a dedicated connector module or a dedicated method with record-specific behavior is a better idea.

Should .like() actually need to be a class method ? Can't it just be a normal method that then can be overloaded by the JTAG class ?

You can override class methods just fine.

Maybe we can solve the Record.like problem by adding a method that returns the arguments to __init__ for that subclass. So for Record itself it would return {"layout": self.layout}, for your subclass it would return {"master": ..., "trst": ...}.

The branch in your example will never fire, unless I'm misreading it.

No, if you call Record.like(jtag), cls will be Record and other.__class__ will be JTAG. Or am I missing something ?

Maybe we can solve the Record.like problem by adding a method that returns the arguments to __init__ for that subclass. So for Record itself it would return {"layout": self.layout}, for your subclass it would return {"master": ..., "trst": ...}.

and then use other.__class__(**kwargs) ? Could be solution.

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 Record, which I was going to do soon.