Skip to content

cleanup: Improve style, formatting; use flake8, isort #254

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

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Aug 20, 2023

This is a style cleanup, to increase code clarity and consistency, supported by manual inspection, flake8, and isort. Most, but not all, of these changes are to formatting, or to the way imports are sorted and grouped (including removing unused imports and placing groups in the PEP-8 recommended order). I've tried to include only Python code changes that I think can be considered improvements whether or not flake8 and isort continue to be used (though I recommend them). This fully retains black style, as well as the central role of black as the main tool for enforcing and checking style in the project.

This adds flake8, which had a configuration in setup.cfg (and had previously been used with Hound), as well as isort. It configures them for consistency with black and with the overall preexisting style. This includes changing the line length to 88, which black uses. The old pep8 configuration is removed, because pep8 was renamed to pycodestyle, and the old pycodestyle is removed, because it doesn't need to be run directly, since flake8 runs it and passes it an appropriate configuration derived from its own. This sets up flake8 and isort for local use, but it does not add them to CI.

Although flake8 detects many things black does not, they partially overlap in their detection of overly long lines. The black rule is more nuanced, rightly skipping over long strings in test data that would be less clear if split across multiple lines. However, flake8 detects comments and docstrings that should be wrapped, so I didn't turn off that flake8 rule. (Such long strings in tests are the only cases where an inline noqa: suppression was needed.)

Besides tool-recommended changes, these other changes are included:

  • Added the common virtual environment directories venv and .venv to .gitignore.
  • Fixed typos in comments. Made a few clarity/consistency tweaks.
  • Moved a buried module docstring to top-of-file and expanded it.
  • Turned a docstring that was meant as a comment into a comment.
  • Made the use of initial and final newlines in and next to docstrings consistent within files, when doing so also made it more consistent with the overall style used in the project.
  • Moved __all__ above other code (as PEP-8 recommends).
  • Changed one __all__ from tuple to list. __all__ can be any sequence of strings, but the other, more prominent __all__ was a list (and using a list is a much more common practice).
  • Replaced a few relative imports with absolute imports, since the overall style in this codebase is to use absolute imports.
  • Changed import X.Y as Y to from X import Y, which was sometimes also used nearby and which sometimes allowed imports to be consolidated.
  • When import X.Y appeared in X, changed to from X import Y. (Importing a submodule makes it accessible via its parent, so this has a similar effect. The main relevant differences, in this codebase, are to omit the unnecessary X.X bindings, and to make it easier for editors/IDEs and less experienced human programmers to understand what the code is doing.)
  • In select cases where it seemed to noticeably improve clarity, changed import X.Y (outside X) into from X import Y and shortened the code using Y accordingly. (Mostly only when other imports from X were present and being used similarly to Y.)
  • Used raw string literals for all regular expressions.
  • Removed noqa (and applied tool-recommended fixes) in cases where noqa seems to have been added for expediency or to work around a combination of limitations in the old flake8 configuration and old behavior of black (rather than to express a preference that the code be kept exactly as it was).

Notable omissions:

  • Although flake8 and isort are configured to ignore versioneer.py, black must still be called with --extend-exclude=versioneer.py. Customizing black's configuration would require a pyproject.toml file to be added, which I think would be better done separately from the changes here.
  • To limit scope, this does not create any new CI linting jobs. Such jobs would not pass yet, due to the next two omissions. Furthermore, even if flake8 and isort linting is desired, there are choices about how, and if, adding CI jobs for them should be done, which probably don't need to be considered in order to evaluate the changes in this PR.
  • No changes are made to conftest.py, since it may make sense to make deprecation-related non-style changes at the same time. This is to avoid an unnecessary conflict with tests: Migrate from pkg_resources to importlib.resources #252.
  • In test_regression_143, the unused validate variables are kept, because non-stylistic considerations should determine what changes, if any, are to be made there. I've opened cleanup: Remove unused test_regression_143 "validate" mocks #253 separately for that.

This is a style cleanup, to increase code clarity and consistency,
supported by manual inspection, flake8, and isort. Most, but not
all, of these changes are to formatting, or to the way imports are
sorted and grouped (including removing unused imports).

This adds flake8, which had a configuration in setup.cfg and seems
to have previously been used, as well as isort. It configures them
for consistency with black and with the overall preexisting style.
This includes changing the line length to 88, which black uses. The
old pep8 configuration is removed, because pep8 was renamed to
pycodestyle, and the old pycodestyle is removed, because it doesn't
need to be run directly, since flake8 runs it and passes it an
appropriate configuration derived from its own. This commit sets up
flake8 and isort for local use, but it does not add them to CI.

Although flake8 detects many things black does not, they partially
overlap in their detection of overly long lines. The black rule is
more nuanced, rightly skipping over long strings in test data that
would be less clear if split across multiple lines. However, flake8
detects comments and docstrings that should be wrapped, so I didn't
turn off the flake8 rule. (Such long strings in tests are the only
cases where an inline "noqa:" suppression was needed.)

Besides tool-recommended changes, these other changes are included:

- Added the common virtualenv dirs venv and .venv to .gitignore.

- Fixed typos in comments. Made a few clarity/consistency tweaks.

- Moved a buried module docstring to top-of-file and expanded it.

- Turned a docstring that was meant as a comment into a comment.

- Made the use of initial and final newlines in and next to
  docstrings consistent within files, when doing so also made it
  more consistent with the overall style used in the project.

- Moved __all__ above other code (as PEP-8 recommends).

- Changed one __all__ from tuple to list. __all__ can be any
  sequence of strings, but the other, more prominent __all__ was a
  list (and using a list is a much more common practice).

- Replaced a few relative imports with absolute imports, since
  the overall style in this codebase is to use absolute imports.

- Changed "import X.Y as Y" to "from X import Y", which was
  sometimes also used nearby and which sometimes allowed imports to
  be consolidated.

- When "import X.Y" appeared *in X*, changed to "from X import Y".
  (Importing a submodule makes it accessible via its parent, so
  this has a similar effect. The main relevant differences, in this
  codebase, are to omit the unnecessary X.X bindings, and to make
  it easier for editors/IDEs and less experienced human programmers
  to understand what the code is doing.)

- In select cases where it seemed to noticeably improve clarity,
  changed "import X.Y" (outside X) into "from X import Y" and
  shortened the code using Y accordingly. (Mostly only when other
  imports "from X" were present and being used similarly to Y.)

- Used raw string literals for all regular expressions.

- Removed "noqa" (and applied tool-recommended fixes) in cases
  where "noqa" seems to have been added for expediency or to work
  around a combination of limitations in the old flake8
  configuration and old behavior of black (rather than to express a
  preference that the code be kept exactly as it was).

Notable omissions:

- Although flake8 and isort are configured to ignore versioneer.py,
  black must still be called with "--extend-exclude=versioneer.py".
  Customizing black's configuration would require a pyproject.toml
  file to be added, so that is deferred for now.

- To limit scope, this does not create any new CI jobs. (Such jobs
  would also not pass yet, due to the next two omissions.)

- No changes are made to conftest.py, since it may make sense to
  make deprecation-related non-style changes at the same time.

- In test_regression_143, the unused "validate" variables are kept,
  because non-stylistic considerations should determine what
  changes, if any, are to be made there.
@cwacek cwacek merged commit d122ce8 into cwacek:master Sep 14, 2023
@EliahKagan EliahKagan deleted the style branch September 14, 2023 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants