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

Bump to version 1.9.0 #39

Merged
merged 20 commits into from
May 9, 2023
Merged

Bump to version 1.9.0 #39

merged 20 commits into from
May 9, 2023

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Apr 19, 2023

Hello @conda-forge/prospector folks, we are big fans of prospector and have been using it for a while now; unfortunately the noarch one on conda-forge is rather old (1.7.7) and we'd like to move on to support Python=3.11, and use the latest prospector. So I am opening this see maybe I can help with the conda package for 1.9.0 - hope you don't mind me doing so 🍺

Closes #35 #36 #37 #38

Checklist

  • Used a personal fork of the feedstock to propose changes
  • [ ] Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link
Contributor

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 (recipe) and found it was in an excellent condition.

@valeriupredoi
Copy link
Contributor Author

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link
Contributor

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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-webservices
Copy link
Contributor

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 (recipe) and found it was in an excellent condition.

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 19, 2023

OK folks, I have now included all the v1.9.0 up to date packages and their pins, but there is one roadblock: requirements-detector that is only at 0.7 on conda-forge; I have opened a PR to bump the version on their feedstock at conda-forge/requirements-detector-feedstock#18 - but until that gets resolved, any way the >=1.0.3 pin you have can be relaxed? 🍺

@valeriupredoi
Copy link
Contributor Author

OK folks, some progress update wrt requirements-detector - I have identified the pkg that blocks the update of that, and opened prospector-dev/requirements-detector#44 to retire it, also my colleague @zklaus is making requirements-detector noarch in conda-forge/requirements-detector-feedstock#19

Copy link
Contributor Author

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

requirements-detector is now built and updated with version 1.2.1

recipe/meta.yaml Outdated Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor Author

@zklaus @jakirkham - tests pass well now yay! Still a bit of cleanup to be done, will try do it later today from the airport, but we on a good path 😁

Copy link
Contributor Author

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

Cleaning up (on phone, so need to do it line by line, no multiline code suggestions on Android/Chrome)

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor Author

Ok folks, @zklaus @jakirkham @bouweandela I have now cleaned up the recipe (apologies for the clunky set of commits, doing this off my phone), I think we should be good to merge this. The reason why I tag Klaus and Bouwe is that it may be a good idea for all three of us to be repo maintainers here - since we use prospector actively, we could fix things here in good time, and get a bit of the burden off the other maintainers, but I'll leave that up to you @jakirkham - a next PR maybe to add us as maintainers. Cheers 🍺

@valeriupredoi
Copy link
Contributor Author

friendly ping @conda-forge/prospector folks 🍺

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks @valeriupredoi! 🙏

Sorry for the slow reply here

Provided a few minor suggestions below

Wonder if we should sort the requirements here to match upstream (to improve readability) or suggest upstream sort them alphabetically (maybe with optional dependencies at the end of the list). This would simplify the review of changes in dependency constraints (or added/dropped dependencies). Just a thought 🙂

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

Also do you want to add yourself as a maintainer here?

valeriupredoi and others added 5 commits May 9, 2023 14:22
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
@valeriupredoi
Copy link
Contributor Author

@jakirkham great to have your reply and review, mate, very many thanks! I have plugged in the suggested deps changes - good spotting and alignment with upstream! I have also added myself as repo maintainer/codeowner here; about sorting the deps - totally with you on that, but if they don't do it upstream, we should probably just wait on them to first do it there. Can suggest that via a PR sometime, my main priority is to first get this outta the way so we can finally Py311 it 😁

@valeriupredoi
Copy link
Contributor Author

@conda-forge-admin, please rerender

@jakirkham
Copy link
Member

Thanks @valeriupredoi! 🙏

Yeah dependency ordering is not a blocker. Mostly noting since there are a lot of dependencies and it is a little hard atm to compare line-by-line atm with different ordering schemes. So it seems like something we would want to improve for maintainability reasons. Maybe we can track this in an issue

Thanks again for all your hard work here ❤️

@jakirkham jakirkham merged commit cf3f348 into conda-forge:main May 9, 2023
This was referenced May 9, 2023
@valeriupredoi
Copy link
Contributor Author

@jakirkham you are a legend! Great many thanks! I shall open an issue and to upstream suggesting reordering their deps, maybe even a PR if I feel generous - will defo fix it here once they do that upstream. Cheers muchly for all the help here 🍺

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.

2 participants