PR for #551: fix minor flake8 complaints #552
Conversation
black is disabled on ropetest folder, because black's treatment of multi-line string with textwrap.dedent() is horrendous and the ropetest folder uses a lot of textwrap.dedent() and multiline strings. If you want to run black on the ropetest folder, you're going to want to use a specific patch of black, and then adjust black's force-exclude in pyproject.toml. ropetest has been excluded by default so that people don't screw up all the formatting by running normal black on ropetest folder. |
|
@lieryan Thanks for the heads up about black. |
…not any safer than normal property access.
lieryan
added
the
cleanup
rope/base/codeanalyze.py
Outdated
| @@ -181,7 +181,7 @@ def logical_line_in(self, line_number): | |||
| indents = count_line_indents(self.lines.get_line(lineno)) | |||
|
|
|||
| def generate_starts(self, start_line=1, end_line=None): | |||
| for start, end in self.generate_regions(start_line, end_line): | |||
| for start, _end in self.generate_regions(start_line, end_line): | |||
There was a problem hiding this comment.
Having a convention where unused variable have to be prefixed with underscore (e.g. _varname) isn't going to be enforceable unless we run a linter scanning for this as pre-commit or as a Github Actions; it's too easy to miss that during manual code review. I'd rather we either disable this warning in local linter or add at least a Github Actions to enforce this.
There was a problem hiding this comment.
flake8 was the source of the complaints. Is it hard to check commits with flake8?
Similarly, it might be good to add a GitHub action that checks that reindent does nothing on all files, including those in the ropetest directory.
Otoh, B007 is a "bugbear" for good reason. I would be happy to disable it in setup.cfg.
There was a problem hiding this comment.
B007 is is now disabled. We can revisit this question at our leisure.
There was a problem hiding this comment.
reindent does nothing on all files, including those in the ropetest directory.
We have Black running on GitHub Actions for formatting checks. I think that should be sufficient, adding more than one formatter would be undesirable, IMO, as it creates the possibility of conflicting advices from the tools.
|
Hi @edreamleo, I think this is about the right size for a PR to be reviewable, any bigger and this will become hard to review the code. Can we freeze this PR at this stage (so any further commits should only be to address things raised during the review), and split any further fixes as separate PR? |
|
@lieryan I agree that this PR should not be bigger. I am willing to close this PR and start a new PR from scratch with the B007 check disabled. A new PR might be best in the long run if you are uncomfortable with adding leading underscores. It's your call. I'm not sure about my own preferences. I think pylint will catch most blunders re B007, but I wouldn't bet my life on it :-) |
|
@lieryan There is no need for me to create a new PR. It's easy for me to revert specific lines, and the diffs in this PR will confirm that the net changes are as I intended. |
|
@lieryan After further thought, I'd like to delete the added underscores and suppress B007 for now. This will shorten this PR and delay a final decision for (much?) later. |
|
@lieryan The diffs now show no added underscores in loop vars. Imo, this PR should now be good to merge. |
|
@lieryan You're welcome. I am enjoying our collaboration! |
See #551.
B007: Loop control variable 'whatever' not used within the loop body.B011: Do not call assert False. Instead raise AssertionError()B009: Do not call getattr with a constant attribute value.E722: Do not use bare except, specify exception instead.E741: ambiguous variable name 'l'.F541: f-string without any placeholders.NQA101: "# noqa" has no violations.