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

Drag and drop source code and tests for poetry-semver #45

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

valeriupredoi
Copy link
Contributor

@carlio and myself had a discussion in #43 and the consensus was to bring over the code from the now-archived poetry-semver here. This is the PR that does that.

@carlio mate, I still need to check the dependencies that requirements-detector will need from porting the poetry-semver code here, but by-and-large the thing works (provided one has pytest, I ran the tests and they run fine). Cheers muchly for your support 🍺

@carlio
Copy link
Member

carlio commented Apr 20, 2023

Pytest is already a dev dependency for CI so that shouldn't be a problem, and the build pipeline seems to pass so it all looks good to me.

I'll leave it a little as it seems you're still adding changes.

@carlio
Copy link
Member

carlio commented Apr 20, 2023

Oh almost - seems like you haven't run the pre-commit checks - see https://github.com/landscapeio/requirements-detector/actions/runs/4755989224/jobs/8450928169?pr=45

You just need to do pre-commit install and it adds git hooks to automatically run linting and formatters when you commit.

You can also manually run it with pre-commit run --all. It'll probably just reformat some of the code

@valeriupredoi
Copy link
Contributor Author

wonderful, many thanks @carlio 🍺 I should be done now but I am not 100% sure about editing the poetry.lock file - poetry-semver is still listed there, and am not very familiar with poetry myself, is that file recreated dynamically? Also, the precommit check is failing, should I fix that?

@carlio
Copy link
Member

carlio commented Apr 20, 2023

For poetry.lock, I suspect that you just manually deleted the poetry-semver line from the pyproject.toml? In that case run poetry lock and it will update the file with the new version constraints (that is, remove poetry-semver)

It's a bit of a hassle, for future reference the best way to do things is to do poetry remove whatever-dependency and it'll update the lock file at the same time.

@valeriupredoi
Copy link
Contributor Author

cheers! Doing that now 👍

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 20, 2023

OK mate, I ran the regen of the poetry lock file and pre-commit (black and isort and all others passed yay!) - I reckon we're good to go 🍻

@carlio
Copy link
Member

carlio commented Apr 20, 2023

Ok! Seems some files still need formatting but no worries, I will merge anyway because I will need to update the version and do an upload with the new code anyway, so I'll fix any missing pre-commit issues at the same time.

Cheers for the PR and thanks for your understanding about not wanting to add poetry as a dependency!

@carlio carlio merged commit de30db4 into prospector-dev:master Apr 20, 2023
carlio pushed a commit that referenced this pull request Apr 20, 2023
@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 20, 2023

ah nice one, cheers muchly @carlio - I had just pushed the isort-ed tests right when you were merging 😁 Many thanks for this, now with a new version I can go build the conda package, so that prospector can be upped in version too, very cool stuff!

@carlio
Copy link
Member

carlio commented Apr 20, 2023

@valeriupredoi Here is your shiny new 1.2.0 release - https://pypi.org/project/requirements-detector/1.2.0/

@carlio
Copy link
Member

carlio commented Apr 20, 2023

wait never mind yanking that, the tests failed

@valeriupredoi
Copy link
Contributor Author

@valeriupredoi Here is your shiny new 1.2.0 release - https://pypi.org/project/requirements-detector/1.2.0/

holy cow, that was fast! Brilliant, very many thanks, mate! On to conda forge now 😁

@carlio
Copy link
Member

carlio commented Apr 20, 2023

Sorry I should have checked the CI output. The test ran for me locally because I still had the poetry-semver package installed. Gimme a minute!

@valeriupredoi
Copy link
Contributor Author

blast! May fault, sorry, mate - just saw that stray import I never fixed, do you want me to fix it?

@valeriupredoi
Copy link
Contributor Author

it's three of them in

./requirements_detector/poetry_semver/version_range.py:import semver
./requirements_detector/poetry_semver/version_union.py:import semver
./requirements_detector/poetry_semver/version_constraint.py:import semver

@carlio
Copy link
Member

carlio commented Apr 20, 2023

No worries I should have double checked the CI output, I forgot my local environment would not be completely clean.

Anyway I fixed it - just added the semver repo as a dependency and it's all working now.

version 1.2.1 is now on PyPI. Crisis averted :-)

@valeriupredoi
Copy link
Contributor Author

you a legend! Sneaky bugger that one - passed all my tests too but I had forgotten that I had the semver package installed (the python-semver, not the poetry-one) and I should have added to the deps list. Brilliant, great many thanks and apologies for the bit of a scare 😁

@carlio
Copy link
Member

carlio commented Apr 20, 2023

All good, I shall go to bed now content with a new release 👍

@valeriupredoi
Copy link
Contributor Author

sleep well, mate 🌙

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