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

Contributing and tools #812

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ivantodorovich
Copy link
Contributor

πŸ“ Update the Contributing documentation

  • Remove the outdated flit section
  • Mention pre-commit and how to use it
  • Describe how to set-up the development dependencies
  • Describe how to run tests

⬆ Bump ruff pre-commit hook

This also does a minor formatting diff on some files


πŸ€– Add mypy pre-commit hook

With this, it's not needed to install mypy directly, as pre-commit will do it in a separate virtual env


🌱 Run lint in a separate ci job

Before this commit, the linting was done in the test matrix (once per python version). This is not necessary, as the linting is the same for all versions.

Now, the linting is fully delegated to pre-commit, and executed as a separate job.

By doing this we delegate ruff completely to pre-commit, and so the linting script and the ruff requirement for testing is removed.

Copy link

πŸ“ Docs preview for commit 009b463 at: https://5b2166b5.typertiangolo.pages.dev

@ivantodorovich ivantodorovich marked this pull request as draft April 27, 2024 23:34
@svlandeg svlandeg added docs Improvements or additions to documentation repo / tests Involving the CI / test suite p3 labels Apr 29, 2024
Copy link

πŸ“ Docs preview for commit 3e75d7c at: https://976d412f.typertiangolo.pages.dev

@ivantodorovich ivantodorovich marked this pull request as ready for review April 29, 2024 13:27
Before this commit, the linting was done in the test matrix (once per python
version). This is not necessary, as the linting is the same for all versions.

Now, the linting is fully delegated to pre-commit, and executed as a separate
job.
* Remove the outdated `flit` section
* Mention pre-commit and how to use it
* Describe how to set-up the development dependencies
* Describe how to run tests
Copy link

πŸ“ Docs preview for commit 15a1dcd at: https://0b4673db.typertiangolo.pages.dev

@svlandeg
Copy link
Collaborator

svlandeg commented Jun 3, 2024

Hi @ivantodorovich, thanks for these contributions! To get this reviewed and merged more easily, I suggest to break this PR up into atomic bits, each representing one functionality change or improvement.

$ pip install flit

---> 100%
$ pip install --editable .
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does require at least pip 21.3, as it'll otherwise fail with a note about not finding setup.py or setup.cfg. So it might be good to add that as a note here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation p3 repo / tests Involving the CI / test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants