CLN: flake8 cleanup of core.internals #19202
Conversation
jorisvandenbossche
changed the title
Codecov Report
@@ Coverage Diff @@
## master #19202 +/- ##
==========================================
+ Coverage 91.51% 91.55% +0.03%
==========================================
Files 147 147
Lines 48773 48796 +23
==========================================
+ Hits 44637 44675 +38
+ Misses 4136 4121 -15
Continue to review full report at Codecov.
|
pandas/core/internals.py
Outdated
| @@ -1108,7 +1108,8 @@ def check_int_bool(self, inplace): | |||
| # a fill na type method | |||
| try: | |||
| m = missing.clean_fill_method(method) | |||
| except: | |||
| except Exception: | |||
There was a problem hiding this comment.
the entire point of correctly lining is to catch a more specific error
don’t put Execption everywhere with TODO that is not useful
his is why we are not raising on master atm with the latest linter yet
There was a problem hiding this comment.
Do you know the appropriate exceptions to catch? I was going to fix these but didn't know what could be raised, and didn't want to break things.
There was a problem hiding this comment.
you simply put a breakpoint in the exception and run the tests
There was a problem hiding this comment.
Well looking at clean_fill_method the only thing it'll raise is ValueError, so this one I can make specific.
There was a problem hiding this comment.
Ditto for clean_interp_method.
|
How confident are you that we have tests exercising all the exceptions that
get raised?
…On Fri, Jan 12, 2018 at 6:11 AM, Jeff Reback ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pandas/core/internals.py
<#19202 (comment)>:
> @@ -1108,7 +1108,8 @@ def check_int_bool(self, inplace):
# a fill na type method
try:
m = missing.clean_fill_method(method)
- except:
+ except Exception:
you simply put a breakpoint in the exception and run the tests
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#19202 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIvX00sUJjLJ5wgh4qOfyryRDzPPRks5tJ0wHgaJpZM4Rb3Mh>
.
|
|
its likeley to be This is why just making these |
|
The only one left is Any idea what the failure mode here is? |
pandas/core/internals.py
Outdated
| @@ -840,7 +840,7 @@ def setitem(self, indexer, value, mgr=None): | |||
|
|
|||
| transf = (lambda x: x.T) if self.ndim == 2 else (lambda x: x) | |||
| values = transf(values) | |||
| l = len(values) | |||
| vlen = len(values) | |||
There was a problem hiding this comment.
It looks like this is only used in one place (line 858). Is there a reason to make it a variable in this case? Seems like we could just replace the one instance where vlen is used with len(values).
There was a problem hiding this comment.
Sounds good, will change.
| @@ -2807,7 +2806,8 @@ def _astype(self, dtype, copy=False, errors='raise', values=None, | |||
| def __len__(self): | |||
| try: | |||
| return self.sp_index.length | |||
| except: | |||
| except Exception: | |||
There was a problem hiding this comment.
Please read my previous comment.
There was a problem hiding this comment.
I did it was not helpful. you need to specify here.
There was a problem hiding this comment.
I did it was not helpful. you need to specify here.
Of course it wasn't helpful. It was asking for help!
Screw it, closing.
There was a problem hiding this comment.
Any idea what the failure mode here is?
@jreback was the comment @jbrockmendel referred to. Saying it was "not helpful" is driving away contributors.
There was a problem hiding this comment.
@jbrockmendel my point is that saying ping I am ready when you haven't answer my questions is not helpful.
There was a problem hiding this comment.
I spend an enormous amounts of my time reviewing PR's and we have a lot of them. I do except answers to comments that I make.
There was a problem hiding this comment.
I'm guessing these last two comments are about a "ping" over in #19139, where I had answered the question but that answer was ignored.
git diff upstream/master -u -- "*.py" | flake8 --diff