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 deprecated. fixes #239 #367

Merged
merged 3 commits into from
Oct 12, 2022
Merged

remove deprecated. fixes #239 #367

merged 3 commits into from
Oct 12, 2022

Conversation

JanMarvin
Copy link
Owner

This caused spurious warnings in RGui and Visual Studio Code.

@jmbarbone let me know what you think about this (iirc you deprecated names.wbWorkbook). In this PR I simply removed the functions altogether, but we could keep them without the deprecation warning (everything else, printing a message will probably pollute the user console as well). The warnings were most likely thrown by IDEs due to names() being such an incremental part of R.
In addition, should we provide a names.wbWorkbook? With this PR names(wb) will print all the internal function names.

On a side note, we probably should think about a time span for deprecated functions. Do we keep them around until someone finally removes them or for the next dot release or for a time span of x days/weeks/months?

This caused spurious warnings in RGui and Visual Studio Code.
@jmbarbone
Copy link
Collaborator

Looks fine. I guess my use of deprecated here is a little weird since I did that before it was CRAN released. I think it's fine to trim out any of that stuff right now.

I don't see much of a utility in names.wbWorkbook() right now. I feel like there are a lot of potential "names" and it might be better to have specific methods like what we have for sheets ($get_sheet_names()). It's still an {R6} object so the default behavior seems fine.

For future deprecating, I agree, that we should probably wait at least until the next minor release to do so, and note that in the warning message.

@JanMarvin JanMarvin merged commit e594e21 into main Oct 12, 2022
@JanMarvin JanMarvin deleted the gh_issue_239 branch October 12, 2022 05:30
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.

2 participants