add support for isort and --fix to spack style by cosmicexplorer · Pull Request #24071 · spack/spack

cosmicexplorer · 2021-06-01T19:27:21Z

Problem

#23947 adds flake8 import style checking, which is awesome. It would be great however if we could force spack style to fix import ordering itself. isort allows this and is compatible with black.

Solution

Add run_isort() to style.py to execute isort in spack style, and add a pyproject.toml configuration file.
- See this page on custom sections and this page on isort options to understand the content of this config file.
Add isort to our dependencies in unit_tests.yaml.
Add a --fix argument to spack style, which configures isort and black to edit files on the local filesystem instead of printing out a diff.
Add the --show-error-codes argument to mypy so that the user can easily see how to disable the warning with a focused # type: ignore[<error-code>] comment.
- See mypy's documentation on error codes.

Result

We can automatically fix import sorting errors identified via #23947 with isort, and we can automatically apply the black formatter as well with the --fix option to spack style!

lib/spack/spack/cmd/style.py

tgamblin · 2021-07-04T09:15:49Z

@adamjstewart I made this work on develop and will submit a separate PR to fix everything. See what you think of the changes.

I also reworked the output for spack style

We enabled import order checking in #23947, but fixing things manually drives people crazy. This used `spack style --fix --all` from #24071 to automatically sort everything in Spack so PR submitters won't have to deal with it. This should go in after #24071, as it assumes we're using `isort`, not `flake8-import-order` to order things. `isort` seems to be more flexible and allows `llnl` mports to be in their own group before `spack` ones, so this seems like a good switch.

adamjstewart

I agree with dropping flake8-import-order in favor of isort as isort seems more powerful and more popular. It is possible to use both (you can write your own custom style for flake8-import-order) but I don't see any advantage in that. It's already hard enough to ensure that flake8 and black agree on style; no need to add both flake8-import-order and isort to the mix. The only annoyance is that flake8 will bring in any plugins it can find, including flake8-import-order which is in my path. We could disable this explicitly, but I might just stop using it for my projects.

pyproject.toml

lib/spack/spack/cmd/style.py

.github/workflows/unit_tests.yaml

lib/spack/spack/cmd/style.py

pyproject.toml

.github/workflows/unit_tests.yaml

lib/spack/spack/cmd/style.py

adamjstewart

There are still a few things I would change (add color to isort/black), move mypy.ini to pyproject.toml, but those can happen in a follow-up PR.

tgamblin · 2021-07-07T10:23:30Z

Ok found the issue -- mypy was failing due to the type stubs for dateutil.parser not being installed in the macOS tests. Sigh.

dateutil.parser was added as an import for tests in #23212 to parse dates out of CVS log output for the mock CVS repo that the tests build. It had to be made optional in #24484. And here, it seems that even though it was a conditional import, mypy is a static tool and needs all the info everywhere.

It looks like there was some discussion of this in #23212 (comment), and the issue is that CVS dates can obnoxiously have different formats on different systems. I googled around and it looks like they can look like any of these:

date: 2021-07-07 02:43:33 -0700;  ...
date: 2021-07-07 02:43:33;  ...
date: 2021/07/07 02:43:33 -0700;  ...
date: 2021/07/07 02:43:33;  ...

So, slashes or dashes, with or without timezone. There never seems to be a sub-second component.

It takes 6 lines of YAML and some weird test-skipping logic to get python-dateutil and types-python-dateutil installed in all the tests where we need them, but it only takes 4 lines of code to write the minimal date parser we need for CVS, so I just did that instead. This has the added benefit that the CVS tests run anywhere CVS is installed; not just where CVS and dateutil are installed. And we no longer need to worry about dateutil compatibility across python versions.

FYI @alalazo, @eschnett, @scheibelp.

Second thought: I don't see any "Modified file" in the logs

lib/spack/spack/cmd/style.py

addressed

tgamblin · 2021-07-07T18:33:26Z

ok we might actually get this in this time -- thanks @alalazo for the late catch

- [x] Remove flake8-import-order checks, as we only need isort for this - [x] Clean up configuration and requirements

This consolidates code across tools in `spack style` so that each `run_<tool>` function can be called indirecty through a dictionary of handlers, and os that checks like finding the executable for the tool can be shared across commands. - [x] rework `spack style` to use decorators to register tools - [x] define tool order in one place in `spack style` - [x] fix python 2/3 issues to Get `isort` checks working - [x] make isort error regex more robust across versions - [x] remove unused output option - [x] change vestigial `TRAVIS_BRANCH` to `GITHUB_BASE_REF` - [x] update completion

Previous tests of `spack style` didn't really run the tools -- they just ensure that the commands worked enough to get coverage. This adds several real tests and ensures that we hit the corner cases in `spack style`. This also tests sucess as well as failure cases.

`dateutil.parser` was an optional dependency for CVS tests. It was failing on macOS beacuse the dateutil types were not being installed, and mypy was failing *even when the CVS tests were skipped*. This seems like it was an oversight on macOS -- `types-dateutil-parser` was not installed there, though it was on Linux unit tests. It takes 6 lines of YAML and some weird test-skipping logic to get `python-dateutil` and `types-python-dateutil` installed in all the tests where we need them, but it only takes 4 lines of code to write the date parser we need for CVS, so I just did that instead. Note that CVS date format can vary from system to system, but it seems like it's always pretty similar for the parts we care about. - [x] Replace dateutil.parser with a simpler date regex - [x] Lose the dependency on `dateutil.parser`

lib/spack/spack/cmd/style.py

We enabled import order checking in #23947, but fixing things manually drives people crazy. This used `spack style --fix --all` from #24071 to automatically sort everything in Spack so PR submitters won't have to deal with it. This should go in after #24071, as it assumes we're using `isort`, not `flake8-import-order` to order things. `isort` seems to be more flexible and allows `llnl` mports to be in their own group before `spack` ones, so this seems like a good switch.

* fix remaining flake8 errors * imports: sort imports everywhere in Spack We enabled import order checking in #23947, but fixing things manually drives people crazy. This used `spack style --fix --all` from #24071 to automatically sort everything in Spack so PR submitters won't have to deal with it. This should go in after #24071, as it assumes we're using `isort`, not `flake8-import-order` to order things. `isort` seems to be more flexible and allows `llnl` mports to be in their own group before `spack` ones, so this seems like a good switch.

cosmicexplorer mentioned this pull request Jun 1, 2021

Use flake8-import-order to enforce PEP-8 compliance #23947

Merged

cosmicexplorer requested review from adamjstewart, alalazo, tgamblin and trws June 1, 2021 19:32

cosmicexplorer force-pushed the add-isort-support branch from a634290 to a1ca83c Compare June 1, 2021 19:41

cosmicexplorer commented Jun 1, 2021

View reviewed changes

lib/spack/spack/cmd/style.py Show resolved Hide resolved

cosmicexplorer commented Jun 1, 2021

View reviewed changes

lib/spack/spack/cmd/style.py Show resolved Hide resolved

cosmicexplorer force-pushed the add-isort-support branch from a1ca83c to 2d3566c Compare June 1, 2021 19:44

sethrj mentioned this pull request Jun 29, 2021

Add e4s tags #23382

Merged

tgamblin force-pushed the add-isort-support branch from 2d3566c to 1f2fd05 Compare July 4, 2021 08:57

spackbot-app bot added commands flake8 workflow labels Jul 4, 2021

tgamblin mentioned this pull request Jul 4, 2021

imports: sort imports everywhere in Spack #24695

Merged

tgamblin requested a review from sethrj July 4, 2021 09:30

adamjstewart reviewed Jul 4, 2021

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

lib/spack/spack/cmd/style.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/style.py Outdated Show resolved Hide resolved

adamjstewart requested review from haampie and vsoch July 4, 2021 17:31

adamjstewart reviewed Jul 4, 2021

View reviewed changes

.github/workflows/unit_tests.yaml Outdated Show resolved Hide resolved

adamjstewart reviewed Jul 4, 2021

View reviewed changes

lib/spack/spack/cmd/style.py Show resolved Hide resolved

adamjstewart reviewed Jul 4, 2021

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

adamjstewart previously approved these changes Jul 5, 2021

View reviewed changes

.github/workflows/unit_tests.yaml Outdated Show resolved Hide resolved

lib/spack/spack/cmd/style.py Outdated Show resolved Hide resolved

sethrj previously approved these changes Jul 5, 2021

View reviewed changes

tgamblin dismissed stale reviews from sethrj and adamjstewart via a3c7b26 July 6, 2021 00:11

spackbot-app bot added the tests General test capability(ies) label Jul 6, 2021

adamjstewart previously approved these changes Jul 6, 2021

View reviewed changes

tgamblin force-pushed the add-isort-support branch from 5bed9de to 7042417 Compare July 7, 2021 10:17

alalazo previously approved these changes Jul 7, 2021

View reviewed changes

alalazo previously requested changes Jul 7, 2021

View reviewed changes

lib/spack/spack/cmd/style.py Outdated Show resolved Hide resolved

tgamblin force-pushed the add-isort-support branch from 7042417 to 0724cf4 Compare July 7, 2021 18:26

tgamblin previously approved these changes Jul 7, 2021

View reviewed changes

alalazo previously approved these changes Jul 7, 2021

View reviewed changes

adamjstewart previously approved these changes Jul 7, 2021

View reviewed changes

trws previously approved these changes Jul 7, 2021

View reviewed changes

cosmicexplorer and others added 5 commits July 7, 2021 13:25

style: add support for isort and --fix

8a2b735

style: Move isort configuration to pyproject.toml

eee9a59

- [x] Remove flake8-import-order checks, as we only need isort for this - [x] Clean up configuration and requirements

tgamblin dismissed stale reviews from trws, adamjstewart, alalazo, and themself via a4082bf July 7, 2021 20:53

tgamblin force-pushed the add-isort-support branch from 0724cf4 to a4082bf Compare July 7, 2021 20:53

adamjstewart reviewed Jul 7, 2021

View reviewed changes

lib/spack/spack/cmd/style.py Show resolved Hide resolved

tgamblin approved these changes Jul 7, 2021

View reviewed changes

sethrj mentioned this pull request Jul 7, 2021

hypre: Add new versions #24625

Merged

tgamblin disabled auto-merge July 8, 2021 00:27

tgamblin merged commit a226862 into spack:develop Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for isort and --fix to spack style#24071

add support for isort and --fix to spack style#24071
tgamblin merged 5 commits intospack:developfrom
cosmicexplorer:add-isort-support

cosmicexplorer commented Jun 1, 2021 •

edited by adamjstewart

Loading

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Jul 4, 2021

Uh oh!

adamjstewart left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart left a comment

Uh oh!

tgamblin commented Jul 7, 2021 •

edited

Loading

Uh oh!

Uh oh!

tgamblin commented Jul 7, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

cosmicexplorer commented Jun 1, 2021 • edited by adamjstewart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Result

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Jul 4, 2021

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Jul 7, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cosmicexplorer commented Jun 1, 2021 •

edited by adamjstewart

Loading

tgamblin commented Jul 7, 2021 •

edited

Loading