-
-
Notifications
You must be signed in to change notification settings - Fork 792
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
add support for packaging #756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a friendly unsolicited review.
I believe there is another pending PR aiming to resolve the issue (just FYI)
"modules", | ||
] | ||
|
||
[tool.poetry] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that familiar w/ poetry to make any strong suggestions here. However, this file doesn't look right from DRY perspective. Essentially, you have pretty much the same contents for both [project] and [tool.poetry] sections. Is there a way to avoid that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poetry is a relatively new and clean package manager for Python which comes with several advantages over pip and it is using pyproject.toml - my problem with accepting both PRs is that they are not properly tested and perhaps submitted without proper understanding of the Poetry concepts and avoiding repetition - thanks for your review and pointing our the DRY principle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my problem with accepting both PRs is that they are not properly tested and perhaps submitted without proper understanding of the Poetry concepts and avoiding repetition
I see what you're saying. I may be able to look into consolidating and testing these PRs code to resolve your concerns this/next week.
// thanks for the link!
] | ||
|
||
[project.optional-dependencies] | ||
test = ["tests"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like an optional dependency to me.
"Programming Language :: Python :: 3", | ||
"License :: OSI Approved :: Apache-2.0 license", | ||
"Operating System :: OS Independent", | ||
"Development Status :: 5 - Production/Stable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It deserves the security topic:
"Development Status :: 5 - Production/Stable", | |
"Development Status :: 5 - Production/Stable", | |
"Topic :: Security", |
You can find more here -- https://pypi.org/pypi?%3Aaction=list_classifiers
Thank you very much for the review! I'm sorry I overlooked that PR. I will just wait for the response from that PR before making any further changes on my side |
No longer relevant, the workflow has been implemented in bdf281c |
Checklist
Changes proposed in this pull request
for issue #633
Your development environment
Ubuntu(WSL)
22.04
3.10.12