-
Notifications
You must be signed in to change notification settings - Fork 78
Make simplify use mostly keyword-only args #851
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
5b0eb43 to
65c7910
Compare
|
@hyanwong I just force-pushed to add tests for this and the other kwarg only methods. This is mainly so we don't add positional arguments in error in future. |
65c7910 to
4000c37
Compare
|
Do we also want to make |
4000c37 to
f84705e
Compare
|
I've added the kw-only stuff to |
jeromekelleher
left a comment
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.
Looks good. I'm slightly nervous about making tracked_samples kw-only, though, as I have a feeling there's a fair bit of coding using that. Are we sure all the documentation examples are ok with this?
I guess tsinfer is a good yardstick - if we don't break any code in there in things like the validation suite, then maybe it's OK. Still, breaking peoples code gratuitously for the sake of neatness isn't good.
I could shift the asterisk so that |
This is probably safer, I think. No point in breaking code for the sake of it. |
f84705e to
aca2338
Compare
Done, with new changelog. Ready to merge, I think. |
|
Great, thanks. It would be nice to test that |
Added |
aca2338 to
17cad82
Compare
benjeffery
left a comment
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.
LGTM
17cad82 to
fe9801b
Compare
|
Just as I was about to merge I noticed the tests were not quite right, fixed and force pushed. |
Fixes #846