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

Replace Tox with Nox #861

Merged
merged 46 commits into from
Mar 28, 2024
Merged

Replace Tox with Nox #861

merged 46 commits into from
Mar 28, 2024

Conversation

IvanIsCoding
Copy link
Collaborator

@IvanIsCoding IvanIsCoding commented Apr 14, 2023

Closes #852

We replace Tox with Nox, overall it seems promising. I do think customizing Nox seems much, much easier given that we have the full power of Python. In fact, I feel we could profit and replace many items in tools/ too with Nox in the future

@IvanIsCoding IvanIsCoding requested a review from mtreinish April 14, 2023 18:10
@IvanIsCoding IvanIsCoding changed the title [WIP] Replace Tox with Nox Replace Tox with Nox Apr 14, 2023
Copy link
Collaborator Author

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

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

Some minor details we need to handle before merging this. But overall, it shouldn't be too hard

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
.flake8 Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 14, 2023

Pull Request Test Coverage Report for Build 8470603505

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 96.514%

Totals Coverage Status
Change from base Build 8468454020: 0.01%
Covered Lines: 17138
Relevant Lines: 17757

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Sorry for the super slow review on this, I'm super keen to get us off of tox. I took a quick look through the config and it looks reasonable to me and this looks great, and is much nicer than what we had with tox. I still need to checkout the branch locally and play with it to see how it works in practice for me.

The one thing I think we should avoid doing for this commit is to keep the tox.ini file around for a little bit. Just because we've been using it since the creation of retworkx so leaving the config around for a while and maybe adding python -c print("The use of tox for running tests is no longer supported in a future version of rustworkx you should use nox instead. Refer to the contributing guide for more details on it's usage") to the tox jobs. I'm just worried about the occasional contributor that comes back after 5 months and tries to run tox and gets frustrated.

.flake8 Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@IvanIsCoding
Copy link
Collaborator Author

Sorry for the super slow review on this, I'm super keen to get us off of tox. I took a quick look through the config and it looks reasonable to me and this looks great, and is much nicer than what we had with tox. I still need to checkout the branch locally and play with it to see how it works in practice for me.

The one thing I think we should avoid doing for this commit is to keep the tox.ini file around for a little bit. Just because we've been using it since the creation of retworkx so leaving the config around for a while and maybe adding python -c print("The use of tox for running tests is no longer supported in a future version of rustworkx you should use nox instead. Refer to the contributing guide for more details on it's usage") to the tox jobs. I'm just worried about the occasional contributor that comes back after 5 months and tries to run tox and gets frustrated.

I'll delete the tox.ini file and add a "legacy" tox.ini to pyproject.toml with the error messages you said

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I pushed up a small commit to add back the tox.ini file in it's previous form and add the warning to the end of the ouptut. I think it's better to not actively break developer's existing workflows, but just make it explicit we're not supporting it anymore. The cost of keeping the tox.ini file around in the short to medium term is minimal.

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Mar 28, 2024
@mergify mergify bot merged commit f93e1b2 into Qiskit:main Mar 28, 2024
30 checks passed
@IvanIsCoding
Copy link
Collaborator Author

I pushed up a small commit to add back the tox.ini file in it's previous form and add the warning to the end of the ouptut. I think it's better to not actively break developer's existing workflows, but just make it explicit we're not supporting it anymore. The cost of keeping the tox.ini file around in the short to medium term is minimal.

I am thinking about wrap the current workflows around nox them i.e. make tox only install nox and then call nox -e test for example

@IvanIsCoding IvanIsCoding deleted the swap-nox-tox branch June 25, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate tox alternatives
3 participants