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

add support for packaging #756

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

Conversation

diamant3
Copy link

Checklist

  • I have followed the Contributor Guidelines.
  • The code has been thoroughly tested in my local development environment with flake8 and pylint.
  • The code is Python 3 compatible.
  • The code follows the PEP8 styling guidelines with 4 spaces indentation.
  • This Pull Request relates to only one issue or only one feature
  • I have referenced the corresponding issue number in my commit message
  • I have added the relevant documentation.
  • My branch is up-to-date with the Upstream master branch.

Changes proposed in this pull request

for issue #633

Your development environment

  • OS: Ubuntu(WSL)
  • OS Version: 22.04
  • Python Version: 3.10.12

Copy link
Collaborator

@arkid15r arkid15r left a 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]
Copy link
Collaborator

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?

Copy link
Collaborator

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.

https://blogs.sap.com/2022/05/08/why-you-should-use-poetry-instead-of-pip-or-conda-for-python-projects/

Copy link
Collaborator

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"]
Copy link
Collaborator

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",
Copy link
Collaborator

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:

Suggested change
"Development Status :: 5 - Production/Stable",
"Development Status :: 5 - Production/Stable",
"Topic :: Security",

You can find more here -- https://pypi.org/pypi?%3Aaction=list_classifiers

@diamant3
Copy link
Author

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

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.

None yet

3 participants