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

Switch out tox for nox #2857

Closed
wants to merge 1 commit into from
Closed

Switch out tox for nox #2857

wants to merge 1 commit into from

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Feb 2, 2022

Description

Towards #2238

It's nicer to work with especially as it uses a bog standard Python file
for configuration. Its output is cleaner and it doesn't hide any magic
too.

Also the mypy cache has been reenabled since it speeds up pre-commit
hooks significantly which makes life easier :)

Hopefully this makes the initial set up process relatively painfree.

Preview: https://black.readthedocs.io/en/hello-nox/contributing/the_basics.html

Checklist - did you ...

  • Add a CHANGELOG entry if necessary? -> n/a
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

It's nicer to work with especially as it uses a bog standard Python file
for configuration. Its output is cleaner and it doesn't hide any magic
too.

Also the mypy cache has been reenabled since it speeds up pre-commit
hooks significantly which makes life easier :)

Hopefully this makes the initial set up process relatively painfree.
@ichard26 ichard26 added skip news Pull requests that don't need a changelog entry. C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases labels Feb 2, 2022
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

diff-shades reports zero changes comparing this PR (03ebaa0) to main (fb9fe6b).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

This sounds great! Here's a few pieces of feedback, mostly typos.

```

### News / Changelog Requirement
You can also setup pre-commit to automatically run before committing. First, install
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can also setup pre-commit to automatically run before committing. First, install
You can also set up pre-commit to automatically run before committing. First, install

```

### News / Changelog Requirement
You can also setup pre-commit to automatically run before committing. First, install
pre-commit similarly to nox and install the Git pre commit hook:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pre-commit similarly to nox and install the Git pre commit hook:
pre-commit similarly to nox and install the Git pre-commit hook:

- `Black` is now more awesome (#X)
### Running tests

Before opening a pull request that involves code, please run the test suite to verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm actually not sure we should recommend this. Local tests are often annoying and finicky to set up, so why not just rely on our CI?


### Style Changes
By default, the test suite is ran with coverage and parallelization enabled, but you can
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
By default, the test suite is ran with coverage and parallelization enabled, but you can
By default, the test suite is run with coverage and parallelization enabled, but you can

$ nox -s docs
```

If you are making many changes to docs, you may find it helpful to use the `docs-live`
Copy link
Collaborator

Choose a reason for hiding this comment

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

this sounds awesome

Comment on lines +140 to +143
Note that X should be your PR number, not issue number! To workout X, please use
[Next PR Number](https://ichard26.github.io/next-pr-number/?owner=psf&name=black). This
is not perfect but saves a lot of release overhead as now the releaser does not need to
go back and workout what to add to the `CHANGES.md` for each release.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Note that X should be your PR number, not issue number! To workout X, please use
[Next PR Number](https://ichard26.github.io/next-pr-number/?owner=psf&name=black). This
is not perfect but saves a lot of release overhead as now the releaser does not need to
go back and workout what to add to the `CHANGES.md` for each release.
Note that X should be your PR number, not issue number! To work out X, please use
[Next PR Number](https://ichard26.github.io/next-pr-number/?owner=psf&name=black). This
saves a lot of release overhead as now the releaser does not need to
go back and work out what to add to the `CHANGES.md` for each release.

No need to apologize here

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, wouldn't it be worth implementing this as a pre-commit script or GHA ? (not necessarily in this PR) "If the diff in CHANGES.md is a single added line, then add (#PR) at the end."
(depending on if you decide on a different path on #2766)

@@ -77,4 +153,9 @@ any large feature, first open an issue for us to discuss the idea first.
## Finally

Thanks again for your interest in improving the project! You're taking action when most
people decide to sit and watch.
people decide to sit and watch. If you ever need help feel free to ask on the relevant
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
people decide to sit and watch. If you ever need help feel free to ask on the relevant
people decide to sit and watch. If you ever need help, feel free to ask on the relevant

Comment on lines +10 to +11
Most development tasks are automated by [nox][nox] which automatically creates virtual
environments and running the right commands. You'll most likely run the test suite,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Most development tasks are automated by [nox][nox] which automatically creates virtual
environments and running the right commands. You'll most likely run the test suite,
Most development tasks are automated by [nox][nox], which automatically creates virtual
environments and runs the right commands. You'll most likely run the test suite,

(.venv)$ pip install -r test_requirements.txt
(.venv)$ pip install -e .[d]
(.venv)$ pre-commit install
(.venv)$ pip install -e .[d,jupyter]
Copy link
Contributor

Choose a reason for hiding this comment

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

What about creating a requirements.txt file that would contain -e .[d,jupyter], so that new devs wouldn't have to track the extras ?
Also, if we don't do that, then:

Suggested change
(.venv)$ pip install -e .[d,jupyter]
(.venv)$ pip install -e '.[d,jupyter]'

The simple quotes are much more likely to work, bash/zsh will likely try to do some completion magic otherwise.

Comment on lines +140 to +143
Note that X should be your PR number, not issue number! To workout X, please use
[Next PR Number](https://ichard26.github.io/next-pr-number/?owner=psf&name=black). This
is not perfect but saves a lot of release overhead as now the releaser does not need to
go back and workout what to add to the `CHANGES.md` for each release.
Copy link
Contributor

Choose a reason for hiding this comment

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

That said, wouldn't it be worth implementing this as a pre-commit script or GHA ? (not necessarily in this PR) "If the diff in CHANGES.md is a single added line, then add (#PR) at the end."
(depending on if you decide on a different path on #2766)

@ichard26
Copy link
Collaborator Author

ichard26 commented Aug 3, 2022

I'm rewriting the developer getting started document so I'll rewrite this PR too. This is pretty stale now.

@ichard26 ichard26 closed this Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants