-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
REGR: exceptions not caught in _call_map_locations #34113
REGR: exceptions not caught in _call_map_locations #34113
Conversation
@@ -509,7 +509,7 @@ cdef class PeriodEngine(Int64Engine): | |||
cdef _get_index_values(self): | |||
return super(PeriodEngine, self).vgetter() | |||
|
|||
cdef void _call_map_locations(self, values): | |||
cpdef _call_map_locations(self, values): |
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.
these are still cdef on master, why is this the appropriate fix on 1.0.4?
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.
why is this the appropriate fix on 1.0.4?
#29232 was a clean-up PR which caused a regression. this is reverting those changes. (applied directly to 1.0.x branch and not touching master, see #34048 (comment))
these are still cdef on master
that's my question to you in the OP.
Could there still be issue on master with the exceptions not being caught?
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.
Could there still be issue on master with the exceptions not being caught?
I can't reproduce the bugs from #32409. I'm guessing you bisected to find the commit that introduced (reintroduced?) the bug, did you find what commit fixed it?
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.
yep fixed on master, see #32409 (comment)
but not sure whether the PR that fixed it #31294 should be backported. I can't immediately see the relation.
I see this PR as the safer option as it's a simple revert.
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 see this PR as the safer option as it's a simple revert.
Works for me
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
NOTE: issue 'fixed' on master. This PR against 1.0.x branch
cc @jbrockmendel Could there still be issue on master with the exceptions not being caught?