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
Adding deprecation warning to explain_prediction. #1860
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1860 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 260 260
Lines 20832 20838 +6
=========================================
+ Hits 20826 20832 +6
Misses 6 6
Continue to review full report at Codecov.
|
33e008f
to
7a0687a
Compare
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!
Do we have an issue filed for removing explain_prediction
yet? :O
@@ -25,6 +25,11 @@ Release Notes | |||
* Pin graphviz version for windows builds :pr:`1847` | |||
* Unpin graphviz version for windows builds :pr:`1851` | |||
|
|||
.. warning:: | |||
|
|||
**Breaking Changes** |
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.
This is only listed as a breaking change but not in the other sections of the release notes? :O Is this intentional 👀
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.
I originally had it in both but then felt it was redundant. Do we have a guideline in place for this?
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.
To date, we've listed in both places. And a single PR could contain multiple lines of change in release notes. However for something this small, I think this is fine.
@angela97lin Yes! We have #1832 |
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.
Nice!
@freddyaboulton Amazing, didn't realize it was filed a while ago. Thank you! |
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.
🚢
7a0687a
to
6318ba1
Compare
Passing RTD build here. |
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!
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.
I really love the simple PRs.
|
Pull Request Description
Fixes #1822
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.