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

[REF-2306] Include twine in dependencies on pyproject.toml #2895

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

martinxu9
Copy link
Contributor

No description provided.

Copy link

linear bot commented Mar 21, 2024

@martinxu9 martinxu9 requested review from Lendemor and picklelo March 21, 2024 16:47
try:
import twine # noqa: F401 # type: ignore
except (ImportError, ModuleNotFoundError) as ex:
if not _pip_install_on_demand("twine"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not "twine>=4.0.0" here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If a twine below 4.0.0 is already installed we won't enter the except

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we kinda touched on this in the standup. I feel the main reason we wanted to do like this is for the installed payload from reflex. Twine is not a big package (not sure how to measure it), but the files in the site-packages directory seem to only mount to around a couple 100KBs. That's not a good enough reason to dance around an already established mechanism of specifying dependencies. Please feel free to share more thoughts. I guess some of the earlier messages likely preceded our standup discussion.

@martinxu9 martinxu9 merged commit 1e3e1df into main Mar 21, 2024
62 checks passed
@martinxu9 martinxu9 deleted the mx/mv-cc-dep-toml branch March 21, 2024 19:48
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.

3 participants