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

feat: Add plothist #27414

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Aug 28, 2024

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • [N/A] If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/plothist/meta.yaml) and found some lint.

Here's what I've got...

For recipes/plothist/meta.yaml:

  • The following maintainers have not yet confirmed that they are willing to be listed here: cyrraz, 0ctagon. Please ask them to comment on this PR if they are.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/plothist/meta.yaml) and found some lint.

Here's what I've got...

For recipes/plothist/meta.yaml:

  • The following maintainers have not yet confirmed that they are willing to be listed here: cyrraz. Please ask them to comment on this PR if they are.

Comment on lines +31 to +33
# Needed for install_latin_modern_fonts
- wget
- unzip
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Should we wait for cyrraz/plothist#176 to be merged and a new small release? Or is it ok to be merged now?

Copy link
Member Author

@matthewfeickert matthewfeickert Aug 28, 2024

Choose a reason for hiding this comment

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

Conda-forge will create new release PRs automatically on the conda-forge/plothist-feedstock that will be created once this is merged, so no real need here to wait now. You can just remove those dependencies in the following conda-forge release PR.

Copy link
Member

@0ctagon 0ctagon Aug 29, 2024

Choose a reason for hiding this comment

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

I am not familiar with the workflow of conda-forge. We created the new release https://github.com/cyrraz/plothist/releases/tag/v1.2.6, but not PR got automatically created in https://github.com/conda-forge/plothist-feedstock. Should I do what the readme says (fork + PR) or did we miss a setup that would automatically create the PR?

Copy link
Member Author

@matthewfeickert matthewfeickert Aug 29, 2024

Choose a reason for hiding this comment

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

Once it is on PyPI a PR should get generated a few hours later. It takes some time for the checking scripts to loop through, but there will be a PR open within the next 24 hours.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, it worked perfectly conda-forge/plothist-feedstock#1 😄

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/plothist/meta.yaml) and found it was in an excellent condition.

* c.f. https://github.com/cyrraz/plothist
* Recipe generated with grayskull via 'grayskull pypi plothist'
  and then edited to add additional information.
* Add wget and unzip as 'run' requirements.
build:
entry_points:
- install_latin_modern_fonts = plothist.scripts.install_latin_modern_fonts:install_latin_modern_fonts
noarch: python
Copy link
Member Author

Choose a reason for hiding this comment

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

Failing on Windows for noarch Python projects is allowed given what I've been told by conda-forge core people in the past so this is fine.

Copy link
Member

Choose a reason for hiding this comment

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

In this particular case the package won't be installable on Windows. If that is by design, we can merge this. However, it does seem like this package can be OS-noarch. See https://conda-forge.org/docs/maintainer/knowledge_base/#noarch-packages-with-os-specific-dependencies for more details.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocefpaf In that case, given #27414 (comment) maybe we can wait for @0ctagon to make a patch release with cyrraz/plothist#176 merged and then we can have that patch release be the one that generates the feedstock (so we'd update the version and sha256).

Copy link
Member

Choose a reason for hiding this comment

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

B/c there is a patch already we can:

  • ignore that, merge it, and you'all fix it as soon as there is a new release
  • backport the patch now

I don't think it is worth making it OS-noarch b/c the fix is there already.

Copy link
Member Author

Choose a reason for hiding this comment

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

ignore that, merge it, and you'all fix it as soon as there is a new release

@ocefpaf I'm find with this suggestion. There should be a new patch release on PyPI before the end of the week so it would only be broken on Windows for a few days.

@0ctagon
Copy link
Member

0ctagon commented Aug 28, 2024

I am willing to be listed as a maintainer.

@matthewfeickert
Copy link
Member Author

@conda-forge/help-python, ready for review!

Copy link
Member

@0ctagon 0ctagon left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@ocefpaf ocefpaf merged commit 636c862 into conda-forge:main Aug 28, 2024
3 of 5 checks passed
@matthewfeickert matthewfeickert deleted the feat/add-plothit branch August 28, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants