STY: enable flake8-bugbear #40570
STY: enable flake8-bugbear #40570
Conversation
| @@ -463,7 +463,7 @@ def check_compound_invert_op(self, lhs, cmp1, rhs): | |||
|
|
|||
| # make sure the other engines work the same as this one | |||
| for engine in self.current_engines: | |||
| ev = pd.eval(ex, engine=self.engine, parser=self.parser) | |||
| ev = pd.eval(ex, engine=engine, parser=self.parser) | |||
There was a problem hiding this comment.
I assume engine was supposed to be used here. This breaks some tests for me locally!
There was a problem hiding this comment.
i think you're probably right. if its breaking tests, probably merits its own PR?
There was a problem hiding this comment.
I opened #40748 to keep track of this.
|
nice - does this solve all bugbear errors? If so, do you want to add it as an additional dependency to the flake8 check? pandas/.pre-commit-config.yaml Line 39 in ced764d |
There are still plenty of other warnings. It might be possible to enable bugbear but we would need to disable some rules/add ignore statements (especially when covering the test code). |
|
bugbear could be enabled when the following warnings are ignored: B004, B005, B006, B008, B009, B010, B011, B015. I feel there is something wrong with this test: I honestly do not understand how |
|
|
||
| r1, _ = idx1.get_indexer_non_unique(idx2) | ||
| r1, _ = idx1.get_indexer_non_unique(indexer) |
There was a problem hiding this comment.
I assume that idx2 was intended to be indexer?
| @@ -128,7 +128,7 @@ def has_horizontally_truncated_repr(df): | |||
| return False | |||
| # Make sure each row has this ... in the same place | |||
| r = repr(df) | |||
| for ix, l in enumerate(r.splitlines()): | |||
| for _ in r.splitlines(): | |||
There was a problem hiding this comment.
Is this just testing that r can be split? The for loop is not needed!
| @@ -956,7 +956,7 @@ def test_query_compare_column_type(setup_path): | |||
| with pytest.raises(ValueError, match=msg): | |||
| store.select("test", where=query) | |||
|
|
|||
| for v, col in zip( | |||
| for v, col in zip( # noqa: B007 | |||
There was a problem hiding this comment.
I don't understand why v cannot be removed here.
There was a problem hiding this comment.
best guess is because somewhere down the line the string on L962 is being passed to eval or exec
|
@MarcoGorelli I enabled almost all of bugbear's checks. The current way to handle B008 might decrease performance a bit (could either add ignore statements or have module-level variables). |
twoertwein
changed the title
doc/source/conf.py
Outdated
| @@ -561,7 +561,11 @@ class AccessorCallableDocumenter(AccessorLevelDocumenter, MethodDocumenter): | |||
| priority = 0.5 | |||
|
|
|||
| def format_name(self): | |||
| return MethodDocumenter.format_name(self).rstrip(".__call__") | |||
There was a problem hiding this comment.
why is it complaining about this? this just seems more verbose
There was a problem hiding this comment.
I agree it is more verbose. I put the style changes into separate commits so that they can easily be dropped.
(r)strip removes an arbitrary set of characters. strip("_") is the same as strip("__"). Calling (r)strip with multiple characters might imply to the reader that exactly this substring is removed, which is not correct.
".....ccaaa__caller.__caller".rstrip(".__caller__") == ""
pandas/core/dtypes/common.py
Outdated
| @@ -1651,7 +1651,7 @@ def _is_dtype_type(arr_or_dtype, condition) -> bool: | |||
|
|
|||
| try: | |||
| tipo = pandas_dtype(arr_or_dtype).type | |||
| except (TypeError, ValueError, UnicodeEncodeError): | |||
There was a problem hiding this comment.
is this a leftover-from-py2 thing?
There was a problem hiding this comment.
I don't know. UnicodeEncodeError is a sub-class of ValueError.
pandas/core/ops/common.py
Outdated
| @@ -49,7 +49,7 @@ def _unpack_zerodim_and_defer(method, name: str): | |||
| ------- | |||
| method | |||
| """ | |||
| is_cmp = name.strip("__") in {"eq", "ne", "lt", "le", "gt", "ge"} | |||
There was a problem hiding this comment.
it doesnt make a differnece, but this was intentional to only strip dunders
pandas/tests/plotting/test_series.py
Outdated
| @@ -686,7 +686,7 @@ def test_time_series_plot_color_with_empty_kwargs(self): | |||
| ncolors = 3 | |||
|
|
|||
| _, ax = self.plt.subplots() | |||
| for i in range(ncolors): | |||
| for _ in range(ncolors): | |||
There was a problem hiding this comment.
maybe intended to pss the arg to s.plot?
There was a problem hiding this comment.
The subplot seems to have only one axis. I think this test makes sure that matplotlib cycles through its colors when called multiple times.
|
I like getting rid of unnecessary zip/enumerates, bug things like requiring unused loop variable to be "_" seem annoying |
I will separate the removal of unnecessary statements into a new PR later today. |
|
Would it make sense to have a new PR that enables only a smaller subset of rules that are less controversial? Skip: Personally, I would like to have B007 since it found unused zips/enumerates and weird tests. B005 would also be great: there is a comment in the function that uses |
|
sure happy to have some of these enabled in a non controversial PR then can follow up enabling others |
There are some cases where one of the values of a
zip()was not used. I removed thezip(). If the two sequences forzip()have different lengths, this would change the behavior.