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

Possible improvements to CONTRIBUTING and installation #27

Closed
RondeauG opened this issue Nov 28, 2023 · 4 comments · Fixed by #28
Closed

Possible improvements to CONTRIBUTING and installation #27

RondeauG opened this issue Nov 28, 2023 · 4 comments · Fixed by #28

Comments

@RondeauG
Copy link

  • Cookiecutter version used, if any: 2023-11-28

Description

Liked to my comments in Ouranosinc/xscen#292 and hydrologie/xhydro#50.

  • Where applicable, I'd replace conda with mamba, with maybe a brief explanation of why.
  • In the Pull request instructions:
    • I'd state somewhere that once installed, pre-commit will automatically run whenever you use the git commit command.
    • Currently, the documentation makes it sound like you need to run black, tox, flake8, etc. every time.
      • Which parts of this are really necessary, since we already have pre-commit? For example with the previous version, I never actually had to run black, flake8, tox, etc. manually, since pre-commit would do it for me and prevent me from pushing my changes if it found issues with my code.
      • The only thing that was really needed was to run pytest and even then, I'd add instructions here on how to run the relevant tests only (pytest tests/test_file.py::TestClass::test_function), rather than the whole suite.
      • Those points are especially relevant when automated actions are implemented on Github, which might not always be the case. How do we manage that?
    • Currently, the documentation makes it sound like you need to run make docs every time. This is confusing for repos where we have automated actions for that. How do we also manage that?
    • I'd mention coveralls at some point and write the script for it (--cov-report html --cov .).
@Zeitsperre
Copy link

Given that we use mamba ubiquitously, I can more or less agree with that. However, this might not be the case for much longer: https://www.anaconda.com/blog/conda-is-fast-now. A counter-proposal would be to add an installation step to install conda-libmamba-solver and touch nothing else. If that ever becomes the standard, then we're back to basics.

pre-commit should be better explained, I agree (it should also be shown how to not commit with the pre-commit hooks, for certain situations). We don't typically run checks individually, so pushing pre-commit or $ make lint and $ make black are more reasonable. I can adjust that.

I can re-add the entry explaining how to run a subset of tests (I'm not sure when this was removed).

There's a way to install pre-commit hooks "globally", or rather, to make it so that if git sees that a repo has a pre-commit.config.yml, it will complain if you haven't run pre-commit install. This is beyond the scope of the project, but maybe it's worth mentioning? Beyond that, the need to run tests should be clear from the docs (no passing tests, no merge).

When I'm developing docs-related features/fixes, I typically run $ make docs every 15 minutes. The CI is absolutely necessary as a check, but I wouldn't rely on it when actively developing (needs to rebuild environment every time it is run; very slow). I still think it's a good idea to run locally.

Can add a mention for coveralls, for sure.

@RondeauG
Copy link
Author

RondeauG commented Nov 28, 2023

My issue with If you are editing the docs, compile and open them with: is that it sounds as if the only way to update the documentation is to run make docs, which simply isn't true if automatic actions have been added to the repo.

Something like this would probably be better: Upon pushing your changes to Github, the documentation will automatically be updated to reflect the changes in your pull request. However, if you are actively editing the docs, you can test your changes locally by compiling and opening them with:

However, this supposes automatic actions. It might be better to keep it as is here, but change it in xscen/xhydro.

@RondeauG
Copy link
Author

A collaborator in xhydro (who is rather new to all of this) tried to follow the instructions, and lost multiple hours building a conda environment. So much so that he had to leave it running for the weekend!

I don't really have a strong opinion in the debate of mamba vs conda-libmamba-solver, as long as we can prevent this kind of issue!

@Zeitsperre
Copy link

Agreed. I remember you mentioning that situation. I'll test locally whether conda-libmamba-solver works for our use-cases and, if so, I'll encourage that approach.

It's a bit unintuitive to encourage users to use an alternative management tool (mamba) to normally operate conda. It isn't a one-to-one drop-in replacement for conda either, so I'd expect users who have some base conda experience would already know when to (and when not to) use it.

Zeitsperre added a commit that referenced this issue Nov 29, 2023
Fixes #27 

### What's changed?

- Mention is made to `conda-libmamba-solver` or `mamba` for handling
dependency solving issues.
- The contributing documentation is clearer on a few procedures
- The default setting of bump-my-version is **not** to tag on bump. This
means that when we create new releases, we need to manually tag that
release on the `main` branch. The `bump-version.yml` will still create
development version tags.
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 a pull request may close this issue.

2 participants