Skip to content
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

Update Formatting Tools #182

Merged
merged 10 commits into from
Jun 29, 2022
Merged

Conversation

adam-grant-hendry
Copy link
Contributor

@adam-grant-hendry adam-grant-hendry commented Jun 28, 2022

The purpose of this PR is a series of formatting changes to bring pyvistaqt formatting tools up to date:

Configuration Changes

  • Update minimum python required to 3.7 and trove classifiers to 3.7-3.9 (9c19230)
  • Add .flake8 and .codespellrc configuration files, updating ignore_words.txt too (bd8914a)
  • Add pyproject.toml (ec25ae7)
    • Add black and pydocstyle configuration options to pyproject.toml (matching options in Makefile and those in pyvista repo and adding corresponding support in .isort.cfg)
  • Add pre-commit (including .pre-commit-config.yaml file) (e041a8b)
    • Fix known issues using mypy, isort, black, and pydocstyle with pre-commit (a07f259)
  • Update environment.yml to include mypy dependency (matching pip requirements) (ae09730)
  • Update .pylintrc to match files ignored in Makefile (c68e16d)
  • Align checking between Makefile and pre-commit, placing all options in config files and pointing to those. This ensures the same checks happen across different systems and all config changes happen in one place as a single source of truth. (31af1e5)
  • Add toml to requirements, environment.yml, and ci/azure-pipelines.yml (supports reading configurations from pyproject.toml) (a914a24)

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #182 (7162d28) into main (a84558f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #182   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files           8        8           
  Lines         648      648           
  Branches       90       90           
=======================================
  Hits          630      630           
  Partials       18       18           

Currently, this is only use for configuring linters and formatters (no
build file specifications are set). Configuration options for `black`
and `pydocstyle` are added here in this commit.
This supports conda test/build environments.
Make this match settings specififed in `Makefile`.
All checks are placed in appropriate config files. Test runner scripts
point to these so that there exists a single source of truth for all
configurations and that the tests performed across systems is the same.
This may be adjusted, of course, if different settings must be used on
a per-system basis.
This package supports reading `pyproject.toml`, which is required for
some of our tests.
`pre-commit` is known to override behavior from config files in
certain instances. This commit fixes known issues for:

- `isort`
- `black`
- `mypy`
- `pydocstyle`

For relevant details, see:

isort:
  + https://jugmac00.github.io/blog/isort-and-pre-commit-a-friendship-with-obstacles/
  + PyCQA/isort#885

black:
  + psf/black#1584

mypy/pydocstyle:
  + python/mypy#4008 (comment)
  + https://pre-commit.com/#hooks-pass_filenames
@adam-grant-hendry adam-grant-hendry changed the title Fix Formatting Update Formatting Tools Jun 28, 2022
@adam-grant-hendry
Copy link
Contributor Author

adam-grant-hendry commented Jun 28, 2022

@pyvista/developers This is ready for review. Can you please review? (I will perform style fixes in another PR after this is approved).

@adam-grant-hendry
Copy link
Contributor Author

@adeak @akaszynski @tkoyama010 Actually, the existing formatting errors will cause tests to fail. Please let me know if you would like me to (a) fix them here, or (b) ignore the offending files here and fix them in another PR.

@akaszynski
Copy link
Member

@adeak @akaszynski @tkoyama010 Actually, the existing formatting errors will cause tests to fail. Please let me know if you would like me to (a) fix them here, or (b) ignore the offending files here and fix them in another PR.

I vote ignore mypy on this PR and fix it in a followup. Just disable it within this PR and proceed.

BTW: Good work implementing the pre-commit. Remove mypy and get the checks green and I'll review.

@adam-grant-hendry
Copy link
Contributor Author

@adeak @akaszynski @tkoyama010 Actually, the existing formatting errors will cause tests to fail. Please let me know if you would like me to (a) fix them here, or (b) ignore the offending files here and fix them in another PR.

I vote ignore mypy on this PR and fix it in a followup. Just disable it within this PR and proceed.

BTW: Good work implementing the pre-commit. Remove mypy and get the checks green and I'll review.

Thanks @akaszynski . I'm glad I took your and @adeak 's suggestion. Not only is this cleaner, but I actually learned a lot!

Ignore linting errors in source code in this PR. The purpose of this PR
is to update linting tools. A future PR will correct the errors. This is
done to separate concerns.
@adam-grant-hendry
Copy link
Contributor Author

@akaszynski All checks have passed.

@tkoyama010 tkoyama010 added the maintenance Low-impact maintenance activity label Jun 29, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, as a followup we'll need to work on mypy.

@akaszynski akaszynski merged commit a3886c2 into pyvista:main Jun 29, 2022
@adam-grant-hendry adam-grant-hendry deleted the maint/formatting branch June 29, 2022 00:10
@adam-grant-hendry
Copy link
Contributor Author

LGTM. Thanks, as a followup we'll need to work on mypy.

You got it. I've been practicing 😉 . I'll be sure to link this PR to the next one that cleans up the linting errors we just ignored.

@adeak
Copy link
Member

adeak commented Jun 29, 2022

Sorry, I got here too late. Nice work @adam-grant-hendry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants