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

[REVIEW] Add deprecation warning for Series.set_mask API #8943

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

galipremsagar
Copy link
Contributor

Series.set_mask is more of internal implementation detail that the end-users will not have knowledge about, hence deprecating the API.

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 3, 2021
@galipremsagar galipremsagar self-assigned this Aug 3, 2021
@galipremsagar galipremsagar requested a review from a team as a code owner August 3, 2021 23:14
@galipremsagar
Copy link
Contributor Author

rerun tests

1 similar comment
@galipremsagar
Copy link
Contributor Author

rerun tests

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. Do we have a general policy on removing immediately vs. deprecating? We often making breaking PRs, so not sure how we should weigh that here.
  2. Do we want to indicate when we'll remove this deprecated API? Again, not sure whether we have standards for that.

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Aug 6, 2021

Two questions:

  1. Do we have a general policy on removing immediately vs. deprecating? We often making breaking PRs, so not sure how we should weigh that here.

Nowadays, for Public APIs we usually deprecate in a release and remove in the next consecutive release(s).

  1. Do we want to indicate when we'll remove this deprecated API? Again, not sure whether we have standards for that.

Don't remember ever specifying a solid version we will remove deprecated API in. There were instances we kept deprecated stuff for years 😆 I plan on removing this next immediate release along with any other bunch of deprecations(if any).

@vyasr vyasr self-requested a review August 6, 2021 00:14
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I would like to formalize some of that a bit in the future, will put that up for discussion in the dev guide.

@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Aug 6, 2021
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@ec99bed). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 33ea7de differs from pull request most recent head 7031430. Consider uploading reports for the commit 7031430 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #8943   +/-   ##
===============================================
  Coverage                ?   10.59%           
===============================================
  Files                   ?      116           
  Lines                   ?    19034           
  Branches                ?        0           
===============================================
  Hits                    ?     2017           
  Misses                  ?    17017           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec99bed...7031430. Read the comment docs.

@rapids-bot rapids-bot bot merged commit 7816a3d into rapidsai:branch-21.10 Aug 6, 2021
shwina pushed a commit to shwina/cudf that referenced this pull request Aug 9, 2021
`Series.set_mask` is more of internal implementation detail that the end-users will not have knowledge about, hence deprecating the API.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: rapidsai#8943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants