-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
BUG: DecimalArray and JSONArray that are empty return incorrect results for isna() #21190
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21190 +/- ##
=======================================
Coverage 91.84% 91.84%
=======================================
Files 153 153
Lines 49505 49505
=======================================
Hits 45466 45466
Misses 4039 4039
Continue to review full report at Codecov.
|
Do we target such small fixed to ExtensionArrays for 0.23.1 or 0.24.0 ? |
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. pls add a whatsnew note in 0.24. virtually all extension changes will have to be in 0.24. FYI
This is a test-only change. I think it's fine for 0.23.1, and doesn't need
a release note.
…On Thu, May 24, 2018 at 6:50 AM, Jeff Reback ***@***.***> wrote:
***@***.**** requested changes on this pull request.
lgtm. pls add a whatsnew note in 0.24. virtually all extension changes
will have to be in 0.24. FYI
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#21190 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIt2w8-85f70NIEV6FB7m3lZnCoZPks5t1p6KgaJpZM4UL5vx>
.
|
no it does need a release note, you have technically changed the behvaior. its ok for 0.23.1 but this may get messy, much easier just to put all EA changes in 0.24 |
@jreback I'm in agreement with @TomAugspurger . You write:
The behavior that is changed is in a test class that is not documented. Has it been prior practice to document in whatsnew when bugs in a test class are fixed? If so, then I'm fine to add a whatsnew. I'm indifferent on the version number. Given the disagreement between the two of you, please let me know how you will reconcile on both the version number issue as well as the whatsnew issue, and how to proceed. |
Yep, the "user facing" (meaning "extension author facing") change was indeed only in a test, so no whatsnew needed. |
Tagging this as 0.23.1, just so we don't forget it. When PRs get backported, we can still change it to 0.24.0 if we decided by then |
…ts for isna() (pandas-dev#21190) (cherry picked from commit 6f1f975)
git diff upstream/master -u -- "*.py" | flake8 --diff
Probably for @TomAugspurger to review.