Unignore more style checks #1561
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were surprisingly few places where this check failed. All have been corrected in the same commit as the ignore list update to avoid introducing a commit with known CI failures.
This is a straight readability enhancement, with a very small effect on existing code (three changed instances in two modules). Enforcing this is mostly to help future contributors who are new to Python learn the convention (and hopefully see why `thing not in dictvar` is preferable to `not thing in dictvar`). Existing contributors already use this style instinctively; the few violations of this rule were in very old code.
Of this commit, half the changes are dealing with the exact same pattern across several modules that all need either `quote()` or `unquote()` out of urllib but have to make it Unicode-safe first for use in Python 2 environments. Depending on your point of view, this is a compelling argument either for keeping/extending Sopel's `web` tools, or for dropping Python 2 support ASAP.
One single real mistake aside, this change caught exclusively code that had been commented-out. Commit 5479ed4 should have removed those lines instead of commenting them out. (Had I been maintainer at the time, #1065 would have been rebased before merging, to get rid of the comments entirely.) As for 822727d, I can only theorize that those two lines were committed accidentally. They were never present in uncommented form (except maybe before that commit was, um, committed), and seem to serve no purpose.
These style rules enforce indentation as a multiple of four spaces (E111) and validate that no unexpected indentation is present (E113). Sopel's codebase is fully compliant with these checks already and no fixes are needed—surprisingly. Don't know why we still ignored them.
Some ignored error codes in the flake8 configuration clearly indicated that Sopel's `__future__` imports were meant to be enforced by `checkstyle.sh`, but the package wasn't required anywhere. Requiring `flake8-future-import` in `dev-requirements.txt` and overhauling the ignored error codes in `setup.cfg` led to a very small set of fixes (two files). Our use of future imports is now verified to be consistent (and in line with what the `CONTRIBUTING.md` file says).
|
Merging this means I can get back to the other PR I'm working on (#1402) with confidence that I won't duplicate anything done here. 😁 |
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Somehow, being sick motivated me to work on a tedious patch that I would usually skip in favor of writing something more interesting. But this seems to have been productive!