Skip to content
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

Document TableRow.replace() ? #1503

Closed
hyanwong opened this issue Jun 18, 2021 · 7 comments · Fixed by #1560
Closed

Document TableRow.replace() ? #1503

hyanwong opened this issue Jun 18, 2021 · 7 comments · Fixed by #1560
Milestone

Comments

@hyanwong
Copy link
Member

hyanwong commented Jun 18, 2021

The most common thing to want to do when using e.g. NodeTable.append() is to want to change one variable, e.g. time. At the moment this requires you to

import dataclasses
node_table.append(dataclasses.replace(row, time=new_val))

Is it worth having the syntactic sugar

node_table.append(row.replace(time=new_val))

to avoid having to import dataclasses and remember the syntax? This could be a lightweight method on a TableRow class, but is it worth duplicating the data class functionality here? I think on balance it might be, especially for newbies to the wonderful world of tree sequences (it might also encourage using .append() instead of .add_row(x, y, z)).

@benjeffery
Copy link
Member

As a data point I have typed the second (currently incorrect) version almost every time I've written this piece of code!

@jeromekelleher
Copy link
Member

jeromekelleher commented Jun 19, 2021

I thought we had done this? As well as Yan's point (which I agree with) I wasn't keen on the long-term API dependency on dataclasses implied by the dataclasses.replace(row, time=new_val)) - if we encourage users to do this, our rows must be dataclasses forever.

@hyanwong
Copy link
Member Author

That's a good point too. Looks like we should just implement it then? Simply this?

def replace(self, **kwargs)
    detaclasses.replace(self, **kwargs)

I don't know what the "/" in the replace docs parameter list is though: https://docs.python.org/3/library/dataclasses.html#dataclasses.replace

@grahamgower
Copy link
Member

I don't know what the "/" in the replace docs parameter list is though: https://docs.python.org/3/library/dataclasses.html#dataclasses.replace

That means the first parameter is positional only. I.e., the keyword cannot be used to specify that param.
https://www.python.org/dev/peps/pep-0570/

@benjeffery benjeffery added this to the Python 0.3.7 milestone Jun 21, 2021
@benjeffery
Copy link
Member

I'm clearly losing my mind as I implemented this already: https://github.com/tskit-dev/tskit/blob/main/python/tskit/util.py#L37

But then I'm sure @hyanwong got an error trying to use .replace on slack the other day?

@hyanwong
Copy link
Member Author

I thought I did too. But it works now. Hmmm.

@hyanwong
Copy link
Member Author

Reopening because I think this should be documented somewhere?

@hyanwong hyanwong reopened this Jun 21, 2021
@hyanwong hyanwong changed the title Implement TableRow.replace() ? Document TableRow.replace() ? Jun 21, 2021
@mergify mergify bot closed this as completed in #1560 Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants