-
Notifications
You must be signed in to change notification settings - Fork 11
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
Remove columns argument #81
Conversation
72694dd
to
af3789f
Compare
@Bisaloo feel free to have a look as it was you who kindly raised the Issue but otherwise fairly confident that it's OK to just go ahead and merge. |
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!
Could you please:
- Edit the documentation to highlight that this argument is deprecated.
I also want to take a moment to think about argument order. I don't know if this argument should stay in this position, as it would imply that all the following arguments have to be named. OTOH, if we move it and someone specified it by position in an old script, it will erroneously be passed to id.column
.
Tricky one. Perhaps keep where it is for now (not breaking existing codes) and remove in 0.4.0 given that people have been warned? |
@Bisaloo any further thoughts? If not I’ll go ahead with this:
|
Yes, seems like the least worse option. It'll be slightly more annoying for users but cannot generate any unexpected behaviour. Just a note that a strict semantic versioning adherence would probably warrant a jump to 1.0.0 to remove this argument. |
Co-authored-by: Hugo Gruson <[email protected]>
2e20b56
to
968fb96
Compare
Fixes #73