Skip to content

Conversation

@Saransh-cpp
Copy link
Contributor

@Saransh-cpp Saransh-cpp commented Oct 24, 2022

Resolves #1211

As title!

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@jakirkham
Copy link
Member

Thanks Saransh! 🙏

Should we also remove these instances too?



.tox/

(not sure if the last one might be worth keeping)

@jakirkham jakirkham requested a review from joshmoore October 24, 2022 20:58
@GbotemiB
Copy link
Contributor

Hi @Saransh-cpp and @jakirkham. since support tox has been removed. Can I go ahead to update the docs to build docs with 'make html'

@jakirkham
Copy link
Member

Hi @GbotemiB. Thanks for the offer. Think that is already in Saransh's PR, but if you see somewhere else that needs to be updated please let us know and we can fix that here.

@joshmoore
Copy link
Member

jakirkham requested a review from joshmoore 13 hours ago

I'm a bit torn, to be honest. The tox.ini is definitely not in an ideal state, especially now with everything being so nice and clean thanks to @Saransh-cpp! However, I do like the idea of having one place that encapsulates the various commands that are needed. I could see preparing a second PR which does the opposite, namely to go all-in on tox and then we could compare the two.

@Saransh-cpp
Copy link
Contributor Author

May I also suggest looking into nox? We use nox in most of the Scikit-HEP packages (for example, vector's noxfile), and I personally like how one can specify instructions in pure python, instead of having a .ini file.

I'll be happy to work on either of them (tox.ini or noxfile.py)!

@jakirkham
Copy link
Member

I'm a bit torn, to be honest. The tox.ini is definitely not in an ideal state, especially now with everything being so nice and clean thanks to @Saransh-cpp! However, I do like the idea of having one place that encapsulates the various commands that are needed. I could see preparing a second PR which does the opposite, namely to go all-in on tox and then we could compare the two.

Given how valuable the conda-incubator/setup-miniconda GHA workflow has been in cutting down maintenance, am not eager to replace that with tox (where we have to maintain even more stuff)

The fact that new contributors are stumbling over tox here just makes it that much clear in my mind that it is time to drop it

@joshmoore
Copy link
Member

Definitely agreed on not reproducing the environmental logic within tox. I rarely if ever use tox for running multiple environments. That type of testing I defer to GHA. For me, it's more a matter of having the individual commands that are hard-coded in the workflows available to be run locally. At the moment, we have different sets of commands spread between:

  • tox.ini
  • the docs/ files
  • the .github workflows

If there were a way to reduce that down to 1️⃣, I'd be pretty happy.

@Saransh-cpp Saransh-cpp changed the title Remove tox.ini and tox support Add nox support and remove tox support Oct 29, 2022
@Saransh-cpp
Copy link
Contributor Author

@joshmoore @jakirkham, could you please have a look at the last commit? I have added nox support to address @joshmoore's comments!

I feel like maintaining a noxfile would be much easier than maintaining a tox.ini file. Plus it is all written in Python! I have also added documentation for the same!

@codecov
Copy link

codecov bot commented Oct 29, 2022

Codecov Report

Merging #1219 (c9a0dac) into main (519f7ef) will not change coverage.
The diff coverage is n/a.

❗ Current head c9a0dac differs from pull request most recent head 422e720. Consider uploading reports for the commit 422e720 to get more accurate results

@@           Coverage Diff           @@
##             main    #1219   +/-   ##
=======================================
  Coverage   99.99%   99.99%           
=======================================
  Files          35       35           
  Lines       14136    14136           
=======================================
  Hits        14135    14135           
  Misses          1        1           
Impacted Files Coverage Δ
zarr/hierarchy.py 99.78% <0.00%> (-0.01%) ⬇️
zarr/util.py 100.00% <0.00%> (ø)
zarr/storage.py 100.00% <0.00%> (ø)
zarr/creation.py 100.00% <0.00%> (ø)
zarr/convenience.py 100.00% <0.00%> (ø)

@jakirkham
Copy link
Member

One could run GitHub Actions locally

https://github.com/nektos/act

@joshmoore
Copy link
Member

Thanks for drafting the nox support, @Saransh-cpp. I agree that the Python syntax is a nice improvement, but I'm not sure it warrants the addition of a new tool. Open to other opinions, but I'd err on the side of reverting those changes. I can understand if you'd like to then just get this merged as is, otherwise happy to discuss other options for the unification.

@Saransh-cpp
Copy link
Contributor Author

Completely understandable! Maybe we can then link or document the usage of https://github.com/nektos/act for Zarr?

@jakirkham
Copy link
Member

Documenting how to use act to run Zarr's tests locally sounds reasonable

@joshmoore
Copy link
Member

Thanks all. @Saransh-cpp, if you can revert, then let's go ahead and get this merged. Someone can work on documenting akt, and I might look into unifying the docs & the GHA commands.

@Saransh-cpp Saransh-cpp changed the title Add nox support and remove tox support Remove tox support Nov 1, 2022
@Saransh-cpp
Copy link
Contributor Author

Reverted!

Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you!

@joshmoore joshmoore merged commit 5b9f2ef into zarr-developers:main Nov 1, 2022
@Saransh-cpp Saransh-cpp deleted the rm-tox.ini branch November 1, 2022 16:42
Comment on lines -50 to -51
[flake8]
max-line-length = 100
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that we lost the flake8 configuration here. Adding back in PR ( #1249 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-use tox where possible from GHA workflows

4 participants