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

Remove columns argument #81

Merged
merged 12 commits into from Sep 5, 2023
Merged

Remove columns argument #81

merged 12 commits into from Sep 5, 2023

Conversation

sbfnk
Copy link
Collaborator

@sbfnk sbfnk commented Jul 27, 2023

Fixes #73

@sbfnk sbfnk force-pushed the remove-columns branch 2 times, most recently from 72694dd to af3789f Compare July 28, 2023 09:46
@sbfnk
Copy link
Collaborator Author

sbfnk commented Jul 28, 2023

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

Copy link
Member

@Bisaloo Bisaloo left a 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.

@sbfnk
Copy link
Collaborator Author

sbfnk commented Aug 14, 2023

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?

@sbfnk
Copy link
Collaborator Author

sbfnk commented Aug 25, 2023

@Bisaloo any further thoughts? If not I’ll go ahead with this:

Perhaps keep where it is for now (not breaking existing codes) and remove in 0.4.0 given that people have been warned?

@Bisaloo
Copy link
Member

Bisaloo commented Aug 31, 2023

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?

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.

R/check.r Outdated Show resolved Hide resolved
@sbfnk sbfnk merged commit c66a6bf into main Sep 5, 2023
9 checks passed
@sbfnk sbfnk deleted the remove-columns branch October 19, 2023 09:15
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 this pull request may close these issues.

Documentation for columns argument in check.survey() is not accurate
2 participants