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

Maint/modernize #76

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Maint/modernize #76

wants to merge 28 commits into from

Conversation

bzah
Copy link
Collaborator

@bzah bzah commented Apr 12, 2024

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is just a guideline):

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

[summarize your pull request here]

This PR fixes #69.
The aim is to modernize the tooling surrounding xncml, including but not limited to:

This also adds a .cruft.sjon file coming from the use of cruft with the ouranosinc template.
Cruft can then be used to update the project structure whenever the template is updated.
However, xncml porject already diverged a little from the original template:

  • CHANGES.rst is renamed CHANGELOG.rst
  • docs/usage.rst is superseded by docs/tutorial.ipynb
  • ruff format replaces black

Breaking changes

  • The build system is now using flit instead of setuptools
  • setup.py was removed.
  • version is no longer fetched from git, but is instead hardcoded in xncml/init.py
  • All workflows have been replaced
    • Publishing to TestPyPI and PyPI are now handled via Trusted Publishing
    • Testing uses tox and reports directly to coveralls.io
    • Many other small changes to aid with maintainability

TODO

  • fix or add ignore of the rules for the pre-commit hook issues
  • Add a personal access token named BUMP_VERSION_TOKEN to enable bump-version workflow.
  • Add personal access token named OPENSSF_SCORECARD_TOKEN to enable scoreyard workflow.
  • Register project on Coveralls, let GitHub manage tokens.

Abel Aoun added 4 commits February 6, 2024 17:40
This aligns xncml enum parsing behavior with xarray's
netCDF4 backend behavior.
- migrate builder from setuptools to flint
- migrate doc structure
- move sources to /src/xncml/ dir (was in /xncml)
- migrate mardown to rst
- update cookiecuttered files with relevant xncml info
@bzah bzah requested a review from Zeitsperre April 12, 2024 14:30
@bzah
Copy link
Collaborator Author

bzah commented Apr 12, 2024

This PR adds an AUTHORS.rst file with the information (including public github emails) from the recorded contributors: @andersy005, @huard, @Zeitsperre, @aulemahal, @fmigneault,
If you would prefer to not be mentioned, just comment on this PR I will remove it.

CHANGELOG.rst Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated
Comment on lines 213 to 226
Before preparing an initial release on conda-forge, we *strongly* suggest consulting the following links:
* https://conda-forge.org/docs/maintainer/adding_pkgs.html
* https://github.com/conda-forge/staged-recipes

In order to create a new conda build recipe, to be used when proposing packages to the conda-forge repository, we strongly suggest using the ``grayskull`` tool::

$ python -m pip install grayskull
$ grayskull pypi xncml

For more information on ``grayskull``, please see the following link: https://github.com/conda/grayskull

Before updating the main conda-forge recipe, we echo the conda-forge documentation and *strongly* suggest performing the following checks:
* Ensure that dependencies and dependency versions correspond with those of the tagged version, with open or pinned versions for the `host` requirements.
* If possible, configure tests within the conda-forge build CI (e.g. `imports: xncml`, `commands: pytest xncml`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider some make release-conda or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it's necessary to create a make release-conda as the initial step for the conda-forge release requires manual step to create the conda-forge feedstock and then, when creating new releases, conda-forge should automatically get the pipy release and create a PR on the feedstock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In all honesty, I wrote this to provide some guidance for new maintainers. If it's irrelevant (i.e. the project was already on conda), there's no harm in removing it.

Perhaps this kind of information would be helpful in a separate documentation file specific for maintainers creating the project from the cookiecutter template that can be more easily removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine either way if it feels overkill for this use case.

I personally prefer to push for the make ... approach because it allows abstraction (CI doesn't need updates if the tool changes) and to keep all the logic together in Makefile rather than spreading it around with custom tweak and combinations of various tool commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Zeitsperre I agree, I think this would fit better in a doc/development/release.rst document as it's rather the job of maintainers and not of the developers.

@fmigneault I also agree. However, one place where most tools are duplicated is the pre-commit config.
I wonder if we can tell pre-commit to run a make lint and make format instead of it fetching these steps from other git repositories.
BTW, that's pretty much the approach of javascripts projects using husky, they define the lint, format, etc, commands once in a versioned package.json and in the pre-commit config of husky, they just call these commands.

CONTRIBUTING.rst Show resolved Hide resolved
@bzah
Copy link
Collaborator Author

bzah commented Apr 15, 2024

Many thanks for your review @fmigneault, there are some xncml specific issues indeed, but I think most of your comments should first be applied to the cookiecutter template.
I can fix these issues here, but I think @Zeitsperre might be interested to upstream some of them.

Abel Aoun and others added 3 commits April 15, 2024 17:27
@Zeitsperre
Copy link
Collaborator

@bzah Thanks for the heads-up! I'm still coming back from my vacation, so I'll give this a look as soon as I can.

@fmigneault If you'd like to upstream a few of your changes so that the nearly 10+ projects using it can also benefit from your work, you are more than welcome to open a PR!

@fmigneault
Copy link
Contributor

I'll let @bzah do it. I only did the proposals, but he did all the good work behind these commits 😉

@bzah
Copy link
Collaborator Author

bzah commented Apr 19, 2024

@Zeitsperre no worries, I will be working on this PR and upstream what's relevant in the coming weeks.

Abel Aoun added 2 commits May 3, 2024 14:44
xncml no longer support python 3.8
@bzah
Copy link
Collaborator Author

bzah commented May 8, 2024

It looks like numpy 2.0.0rc1 on python 3.12 breaks some automated tests. I will have a look and try to fix this in another PR.

Abel Aoun added 2 commits May 13, 2024 09:43
Moved the 'how to release' from the contributing guide to
a dedicated releasing.rst guide.
@Zeitsperre
Copy link
Collaborator

@bzah

Once the changes have been merged to the main branch of the template, I think we can fast-forward those changes here and safely merge!

@bzah
Copy link
Collaborator Author

bzah commented May 13, 2024

@Zeitsperre I will try to update this branch via cruft tomorrow. I'm away for a month on Wednesday, so I hope I can get something clean and we can merge this one before I leave.

@Zeitsperre
Copy link
Collaborator

@fmigneault The changes coming from the cookiecutter template are done, IMO. The last tasks needed now require Admin access (setting tokens for the repo and registering the project on outside platforms).

@bzah
Copy link
Collaborator Author

bzah commented Jul 4, 2024

I'm not sure who has the admin rights to setup the tokens. Perhaps, we could disable the use of bumpmyversionm, scoreyard and Coveralls so that this PR can move forward.

@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Jul 5, 2024

@bzah I'm going to fast-forward the cookiecutter to the latest commits to see if we can finalize this. For the token-related issues, we need someone with admin privileges to get that up and running (or to share those privileges so that I can put them in place).

@huard Are you able to set these things up?

Edit: Thanks David!

@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9811258437

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on maint/modernize at 91.278%

Totals Coverage Status
Change from base Build 8659284774: 91.3%
Covered Lines: 2899
Relevant Lines: 3176

💛 - Coveralls

@coveralls
Copy link

Coverage Status

coverage: 91.278%. first build
when pulling 445b7e8 on maint/modernize
into 89a6997 on main.

@Zeitsperre
Copy link
Collaborator

Zeitsperre commented Jul 5, 2024

@bzah and @andersy005 (also @huard)

The last things missing are the personal access tokens. xarray-contrib does not currently allow members to create personal access token requests, so my hands are currently tied, but if this could be enabled, we'll have a much easier time.

Relevant StackOverflow: https://stackoverflow.com/questions/75044318/create-personal-access-token-request-for-organization

Additionally, since I'm not a member of xarray-contrib I'm not sure if I would have the rights to make such a request (but @andersy005 would).

An alternative approach to making a personal access token tied to an individual user is to create a GitHub App to manage this (I've performed this recently for @Ouranosinc: https://github.com/apps/ouranos-helper-bot; It's relatively simple, but the maintainer again would need to be a member of the organization).

This is the last thing blocking this PR. Let me know if you have any questions. Best,

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.

ENH: Migrate to pyproject.toml and ruff
4 participants