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

Move version from const.py to __init__.py for Flit static introspection #165

Merged
merged 3 commits into from
Oct 21, 2023

Conversation

CAM-Gerlach
Copy link
Contributor

As discussed in #164 , moving __version__ from const.py to be directly in __init__,py allows Flit to statically introspect it, preventing requests from being required as an unspecified, implict build-time dependency, causing build/install failures in certain scenarios. I've tested and confirmed locally that this fix allows successfully building and installing prawcore without having requests in the local environment.

Additionally, I've updated the one internal module using this value from const.py to use that from __init__.py. I did have to switch from a relative from import to an absolute import to avoid an import cycle error; alternatively, I could also move the import to be inside the relevant function, though I figured the former would be more acceptable/less problematic for you—happy to change that as desired.

Finally, I updated the version bump script in tools/set_version.py to bump the version in its new location.

@CAM-Gerlach CAM-Gerlach force-pushed the fix-dynamic-version-import branch from 6083cce to 33f8284 Compare October 20, 2023 22:32
@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review October 20, 2023 22:40
Copy link
Member

@LilSpazJoekp LilSpazJoekp left a comment

Choose a reason for hiding this comment

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

Yeah let's do a local import.

@@ -5,7 +5,9 @@

import requests

from .const import TIMEOUT, __version__
import prawcore
Copy link
Member

Choose a reason for hiding this comment

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

Let's do a local relative import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done as requested 👍

@LilSpazJoekp LilSpazJoekp merged commit 972f582 into praw-dev:main Oct 21, 2023
12 checks passed
@LilSpazJoekp
Copy link
Member

Thanks for this!

Would you be interested in doing the same for PRAW, Async PRAW, and Asyncprawcore?

@CAM-Gerlach
Copy link
Contributor Author

Hey, sorry, was looking into this, but ran into a couple issues and then it fell through the cracks.

I opened praw-dev/praw#2004 to cover this in PRAW and discuss the implementation there; TL;DR we could do a similar approach, but it does require changing one other const.py module-level constant in a backward-incompatible way, so we may want to re-consider Approach 2 (static metadata) here, but that can be discussed over there.

As for asyncpraw and asyncprawcore, at the time I first looked into this request they were both still using Setuptools, but I see they are now using Flit, so they do indeed need this too, presumably. Asyncpraw runs into the same issue as PRAW with USER_AGENT_FORMAT and can be fixed the same way, with the same impact (albeit theoretical in this case); however, I note one extant usage of asyncpraw.const.__version__ in the wild, in someone's Reddit bot with a handful of stars. Approach 2 (static metadata) would avoid this as well, as well as simplify the implementation there since it would essentially just require the same changes to set_version.py and pyproject.toml and that's it.

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