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

Housekeeping #11

Merged
merged 26 commits into from
Jun 13, 2023
Merged

Housekeeping #11

merged 26 commits into from
Jun 13, 2023

Conversation

Zeitsperre
Copy link
Collaborator

Hello!

This is a draft PR to update some of the GitHub workflows to use the conventions and regenerate the boilerplate code using https://github.com/Ouranosinc/cookiecutter-pypackage and cruft for versioning of the boilerplate.

There will be many merge conflicts that I will resolve, and xhydro will benefit from an updated/consistent cookiecutter recipe, pre-commit configuration, updated documentation, etc.

I am currently waiting on a PR review before moving forward with those changes: Ouranosinc/cookiecutter-pypackage#20

Questions

  • sphinx-pangeo-theme has been unmaintained for 4+ years and is not used in the docs, are we safe to remove it?
  • I noticed some GitLab files mentioned. Is there a GitLab mirror of this repo?
  • Requirements mirror xclim very closely; Am I free to remove most of these? We can add back in the relevant requirements as code is added to the repo.
  • Are the documentation dependencies listed in environment.yml based on existing notebooks? If they are only anticipated to be useful, can we comment many of these out?

@Zeitsperre Zeitsperre added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 2, 2023
@Zeitsperre Zeitsperre self-assigned this Jun 2, 2023
@Zeitsperre Zeitsperre requested review from RondeauG and TC-FF June 2, 2023 19:04
@Zeitsperre
Copy link
Collaborator Author

Zeitsperre commented Jun 9, 2023

Hello! (PVI @lpcaron)

I was able to update the cookiecutter this week and have regenerated the boilerplate code here. There are still some finishing touches to do on my side, but code reviews are welcome.

There's a lot going on here, but these are the key points:

Changes

This PR adds several new features:

In addition, the boilerplate is now being tracked/versioned via cruft. What this means is that when changes occur to the cookiecutter that this was based on (https://github.com/Ouranosinc/cookiecutter-pypackage), we can then apply those changes directly here to keep things up-to-date.

There were a few small bugs in the cookiecutter recipe that I missed/introduced in my last update, so I will likely be fixing those upstream and then fast-forwarding the changes here (perhaps in another PR, not urgent).

I also have plans to update the cookiecutter to use the more modern pyproject.toml standard (I've mentioned this point in meetings; For more information: PEP 517 and PEP 621, an example from xclim). This is a significant departure from the cookiecutter here (deletion of many files, moving configurations around, etc.), but again, upstream changes will be easily applied to xhydro when they're done. This task was already on my "to-do" list for other projects based on that cookiecutter.

Lastly, I've made a request to the admin of Hydrologie (not sure who that is) to allow for the Coveralls Pro webhook on this repository. Once that is approved, we will be able to monitor code coverage changes on every Pull Request.

All the best,

@Zeitsperre Zeitsperre marked this pull request as ready for review June 9, 2023 21:48
Copy link
Contributor

@sebastienlanglois sebastienlanglois left a comment

Choose a reason for hiding this comment

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

Thanks @Zeitsperre for this amazing work in adapting the repo to modern conventions. We will benefit from this tremendously!

.coveralls.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
.readthedocs.yml Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
@sebastienlanglois
Copy link
Contributor

Questions

  • sphinx-pangeo-theme has been unmaintained for 4+ years and is not used in the docs, are we safe to remove it?
  • I noticed some GitLab files mentioned. Is there a GitLab mirror of this repo?
  • Requirements mirror xclim very closely; Am I free to remove most of these? We can add back in the relevant requirements as code is added to the repo.
  • Are the documentation dependencies listed in environment.yml based on existing notebooks? If they are only anticipated to be useful, can we comment many of these out?
  1. Yes, sphinx-pangeo-theme can be safely deleted as the theme used is "furo".
  2. No there are none
  3. Yes
  4. Yes, the environment.yml is based on existing notebooks and "core" xhydro code that will soon be merged to the master branch.

Copy link
Collaborator

@RondeauG RondeauG left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, great job!

.github/workflows/main.yml Outdated Show resolved Hide resolved
.zenodo.json Outdated Show resolved Hide resolved
@Zeitsperre
Copy link
Collaborator Author

@RondeauG

I need to perform one or two final adjustments (fixing a last bug I added, haha), but this will be ready to be merged early tomorrow.

The pyproject.toml changes will come in another PR sometime soon. No major rush.

@sebastienlanglois
Copy link
Contributor

@Zeitsperre , just realised the CONTRIBUTING.rst has some TC-FF that should be replaced with hydrologie

@RondeauG
Copy link
Collaborator

@RondeauG

I need to perform one or two final adjustments (fixing a last bug I added, haha), but this will be ready to be merged early tomorrow.

The pyproject.toml changes will come in another PR sometime soon. No major rush.

Great! Feel free to merge when you think that it's good to go.

@Zeitsperre Zeitsperre merged commit 46dcb93 into master Jun 13, 2023
5 checks passed
@Zeitsperre Zeitsperre deleted the housekeeping branch June 13, 2023 15:57
This was referenced Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants