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/move to poetry #470

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

martynvdijke
Copy link

Hey all,

Just found this repo while searching for the docs I saw that #464 was still hanging.
So decided to fix that, this introduced poetry as a dependency management tool.
It should also add the pylint configuration as a setting file.

Copy link
Contributor

@github-actions github-actions bot 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 contributing to p5py

@tushar5526
Copy link
Member

Thank you for the PR @martynvdijke. It would be great to use tilde specification to specify the dependency. That way we can still get minor updates for those libraries.

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Author

@martynvdijke martynvdijke left a comment

Choose a reason for hiding this comment

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

Move to use poetry

@tushar5526
Copy link
Member

Thanks @martynvdijke I will review this soon!

@tushar5526
Copy link
Member

@martynvdijke thank you for the PR. This looks great to me, I will test this a bit more locally and merge this soon!

@martynvdijke
Copy link
Author

@tushar5526 any update on this ?

@martynvdijke
Copy link
Author

Hey hey @tushar5526 I guess you are busy, no worry on that any update on the PR ?

@tushar5526
Copy link
Member

Hey @martynvdijke ,sorry as I have not been able to find much time (lots of stuff going on rn) I will try to share feedback with you this weekend. Thank you for your patience :)

@martynvdijke
Copy link
Author

No worries, take your time just checking in :)

pyopengl-accelerate = "^3.1.7"
pyopengl = "^3.1.6"
requests = "^2.25.0"
numpy = [
Copy link
Member

Choose a reason for hiding this comment

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

In the requirements.txt numpy is not tagged to a specific version (which is a bad practice already)

I appreciate you pinning the version, but why different versions are pinned for different python version?

Copy link
Author

@martynvdijke martynvdijke Sep 12, 2024

Choose a reason for hiding this comment

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

This is needed because numpy version 1.21 is the last version that is python3.7 compatible.
Otherwise poetry can not figure out the dependency for python 3.7 if that makes sense ?

So by pinning it like this we can ensure compatibility on python3.7 and have a more up to date version for the python versions above.


[tool.poetry.dependencies]
python = "^3.7"
freetype-py = "^2.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why freetype-py is added a direct dependency?

Copy link
Author

Choose a reason for hiding this comment

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

Removed

skia-python = "^87.5"


[tool.poetry.group.dev.dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

Why different dependency versions are pinned for different python versions?

Copy link
Author

Choose a reason for hiding this comment

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

Same reason as above otherwise poetry can figure out which dependencies to use for older python versions.
Since they cannot run the newest version on the older pytthon versions

@tushar5526
Copy link
Member

@martynvdijke , again thanks for your PR and patience. I have left some comments.

I think we can also remove the requirements.txt file if there are no dependency on it.

@martynvdijke
Copy link
Author

@tushar5526 I had a look at the comments and update and explained to your comments

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