Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Improve tool configuration and usability #586

Closed
Mr-Pepe opened this issue Apr 2, 2022 · 5 comments · Fixed by #588
Closed

Improve tool configuration and usability #586

Mr-Pepe opened this issue Apr 2, 2022 · 5 comments · Fixed by #588

Comments

@Mr-Pepe
Copy link
Contributor

Mr-Pepe commented Apr 2, 2022

#584 started a little discussion on where to put tool configurations (for mypy, pytest, etc).

I would like to propose the following steps to improve the pipeline:

mypy
The mypy configuration has already been moved to pyproject.toml in #584.

  • Change mypy src/ to mypy . in tox.ini (this currently leads to two easy to fix issues)

pytest

  • Move the [pytest] section from tox.ini to a [tool.pytest] section in pyproject.toml
  • Add --cache-clear and -vv to the [tool.pytest] section of pyproject.toml and remove them as command line arguments in tox.ini
  • Move src/tests to tests and change pytest src/tests {posargs} to pytest {posargs} in tox.ini

pydocstyle

  • Move the [py257] section from tox.ini to a [tool.pydocstyle] section in pyproject.toml

Where is this configuration used in the pipeline? I don't see pydocstyle being invoked anywhere.

black

  • Change black --check src/pydocstyle to black --check . in tox.ini

isort

  • Change isort --check src/pydocstyle to isort --check . in tox.ini

The aforementioned changes allow simply running pytest, mypy ., black . and isort . in the project folder. Furthermore, the changes facilitate setting up these tools in an IDE (e.g., format on save in VS Code) without having to specify any options.

@samj1912
Copy link
Member

samj1912 commented Apr 2, 2022

We run pydocstyle on the project at

def test_pep257_conformance():

@aphedges
Copy link
Contributor

aphedges commented Apr 2, 2022

While I am in favor of some of these changes, others will likely either cause problems with tests or unnecessarily large diffs:

  • Black reformats docstrings, so we wouldn't want it to run on some of the files in test. I can't point to specifics at the moment.
  • Moving src/test to test and adding Black and isort to more files will create some diffs that will make the commit history more difficult to read. https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/ will make it easier when using GitHub, and other tooling will need to rely on whatever git blame --ignore-revs-file support exists. In addition, GitHub still doesn't support log --follow yet, so moving files can make it more difficult to follow the history on here.

If we really want an easy and short version to run commands with different scopes from the terminal, we can add targets to the Makefile. We would have make mypy be mypy src while make black would be make src/pydocstyle. It probably wouldn't help much with other programs, but I believe that some (e.g. PyCharm) do support calling a Makefile.

@Mr-Pepe
Copy link
Contributor Author

Mr-Pepe commented Apr 2, 2022

We run pydocstyle on the project at

def test_pep257_conformance():

But that specifies its own configuration, right? I have just checked and we could simply run pydocstyle via its CLI as part of the tox pipeline.

@Mr-Pepe
Copy link
Contributor Author

Mr-Pepe commented Apr 2, 2022

@aphedges Fair points. That leaves:

  • Move the [pytest] section from tox.ini to a [tool.pytest] section in pyproject.toml

  • Add --cache-clear and -vv to the [tool.pytest] section of pyproject.toml and remove them as command line arguments in tox.ini

  • Move the [py257] section from tox.ini to a [tool.pydocstyle] section in pyproject.toml

@aphedges
Copy link
Contributor

aphedges commented Apr 2, 2022

@Mr-Pepe, it looks like different configuration is being used in test_pep257_conformance compared to tox.ini. It's probably okay to just delete the [pep257] section entirely and not replace it with anything else. Keeping the two configurations in sync seems like more trouble than it would be worth.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants