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.

Sign up for GitHub

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

Jump to bottom

Adding h5glance and htmlgen #22647

Merged
merged 13 commits into from
Apr 24, 2023
Merged

Adding h5glance and htmlgen #22647

merged 13 commits into from
Apr 24, 2023

Conversation

tdegeus
Copy link
Member

@tdegeus tdegeus commented Apr 21, 2023

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).
  • 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/h5glance) and found some lint.

Here's what I've got...

For recipes/h5glance:

  • about/license_family 'BSD 3-Clause' not allowed. Allowed families are
    AGPL, APACHE, BSD, CC, GPL, GPL2, GPL3, LGPL, MIT, MOZILLA, NONE,
    OTHER, PROPRIETARY, PSF, and PUBLIC-DOMAIN.
  • noarch: python recipes are required to have a lower bound on the python version. Typically this means putting python >=3.6 in both host and run but you should check upstream for the package's Python compatibility.

For recipes/h5glance:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@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/h5glance) and found some lint.

Here's what I've got...

For recipes/h5glance:

  • noarch: python recipes are required to have a lower bound on the python version. Typically this means putting python >=3.6 in both host and run but you should check upstream for the package's Python compatibility.

For recipes/h5glance:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023

@takluyver do you want me to add you as maintainer ?

@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/h5glance) and found some lint.

Here's what I've got...

For recipes/h5glance:

  • Invalid noarch value python >=3.7. Should be one of python, generic.

For recipes/h5glance:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@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/h5glance) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/h5glance:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@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/h5glance, recipes/htmlgen) and found it was in an excellent condition.

@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023

@srittau Do you want to be added as a maintainer of htmlgen ?

@tdegeus tdegeus changed the title Adding h5glance Adding h5glance and htmlgen Apr 21, 2023
@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023 •
edited
Loading

@conda-forge/help-python The recipe is ready for review.

Windows build failure is expected: the test using grep does not work on Windows. This will not cause any problems as the generic package will be build on Linux.

If @srittau or @takluyver want to be co-maintainer in the future they can be added at any point

@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023

@conda-forge/help-python ready for review

@takluyver
Copy link
Contributor

@takluyver do you want me to add you as maintainer ?

If you're happy maintaining them, I'd rather not - I already ignore quite a lot of emails from conda-forge for packages where I'm listed as a maintainer. But if you'd like another maintainer, I'm happy enough to be it.

@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023

I'm 'happy' to do my community service ;) . If you would not might having a glance now...?

@tdegeus @takluyver
Update recipes/h5glance/meta.yaml
fce75d8 
Co-authored-by: Thomas Kluyver <takowl@gmail.com>
@tdegeus
Copy link
Member Author

tdegeus commented Apr 21, 2023

Thanks @takluyver . A note is that h5glance-html does not have a --version.

@takluyver
Copy link
Contributor

Thanks @takluyver . A note is that h5glance-html does not have a --version.

There I think I can say: PRs welcome 😉

@tdegeus tdegeus mentioned this pull request Apr 21, 2023
Closed
@takluyver
Copy link
Contributor

The recipe otherwise looks good for me.

I don't know if conda has any special support to load bash/zsh completions inside an environment - if it does, it would be good to set up h5glance's completions with that.

@tdegeus
Copy link
Member Author

tdegeus commented Apr 22, 2023

Taking the discussion from gitter here :

@jaimergp
Copy link
Member

You can have platform-specific noarch with this approach.

That said, I am not sure auto-installing autocompletions is a good practice. You could provide a command to make it easy for users (e.g. a simple bash script in bin/), but having system modifications (even if technically reversible) might be seen as strange for some users. My two cents; I don't think we have a policy for this. All I am saying is that maybe you can avoid the activation complexity if you drop the autocomplete integrations :D

@tdegeus
Copy link
Member Author

tdegeus commented Apr 23, 2023

Understood. Since there is an extremely easy command I propose that we go ahead and merge this (after review if necessary)

jaimergp
jaimergp reviewed Apr 23, 2023
View reviewed changes
recipes/h5glance/meta.yaml Outdated
Comment on lines 41 to 49
about:
home: https://github.com/{{ org }}/{{ name }}
license: BSD-3-Clause
license_family: BSD
license_file: LICENSE
summary: Lets you explore HDF5 files in the terminal or an HTML interface.
description: Lets you explore HDF5 files in the terminal or an HTML interface.
doc_url: https://github.com/{{ org }}/{{ name }}
dev_url: https://github.com/{{ org }}/{{ name }}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not use Jinja here so humans can copy paste URLs from the recipe directly. Makes it more difficult to parse. It's not like that org or repo names will change often anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine. I developed this habit to avoid copy/past mistakes for new feedstock, but I will change

@jaimergp
Copy link
Member

Do you want to include the tests for htmlgen? They are being bundled now but I don't know if accidental or not.

tdegeus
tdegeus commented Apr 23, 2023
View reviewed changes
recipes/htmlgen/meta.yaml Outdated Show resolved Hide resolved
@tdegeus
Copy link
Member Author

tdegeus commented Apr 23, 2023

Do you want to include the tests for htmlgen? They are being bundled now but I don't know if accidental or not.

I don't know why, but they are specifically included upstream it seems

https://github.com/srittau/python-htmlgen/blob/58930b8bb76723d40dc09c27522e2ed73e745e8c/pyproject.toml#L16

I would prefer not to make a patch. Is there any other way to remove them ?

tdegeus
tdegeus commented Apr 23, 2023
View reviewed changes
recipes/h5glance/meta.yaml Outdated Show resolved Hide resolved
tdegeus
tdegeus commented Apr 23, 2023
View reviewed changes
recipes/htmlgen/meta.yaml Outdated Show resolved Hide resolved
@jaimergp
Copy link
Member

That's ok, just wanted to bring that your attention. They are not in "tests/", so that's good enough.

@jaimergp jaimergp merged commit a1a91c0 into conda-forge:main Apr 24, 2023
@tdegeus tdegeus deleted the h5glance branch April 24, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takluyver takluyver takluyver left review comments

@jaimergp jaimergp jaimergp approved these changes

Assignees
No one assigned
Labels
python review-requested
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tdegeus @takluyver @jaimergp