-
Notifications
You must be signed in to change notification settings - Fork 47
Add a version attribute #81
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
Conversation
Can you lease describe the modifications? |
@eolivelli thanks, i just updated the description in the parent comment to describe what is being done! |
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.
LGTM. This appears to follow this paradigm: https://packaging.python.org/en/latest/guides/single-sourcing-package-version/.
Note: I am not formally "approving" because I do not know enough about Python's standards here.
pulsar/__init__.py
Outdated
@@ -62,6 +62,11 @@ | |||
from datetime import timedelta | |||
|
|||
|
|||
# Get the pulsar version from version.txt | |||
with open("version.txt") as f: |
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 believe you should adjust this line based on the __file__
path.
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.
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.
The root cause is version.txt
is not included in the Python wheel. I just checked the installed wheel by this patch:
$ ls /usr/local/lib/python3.8/dist-packages/pulsar
__init__.py __pycache__ exceptions.py functions schema
This PR looks like to use the 4th method of https://packaging.python.org/en/latest/guides/single-sourcing-package-version/#single-sourcing-the-version, but as the warning says:
The If I were you, I would adopt the 3rd method, which is simple and clear. Create an __version__='3.1.0a1' Then, in from pulsar.__about__ import __version__ And in def get_version():
version = {}
with open("./pulsar/__about__.py") as fp:
exec(fp.read(), version)
return version['__version__'] References: |
@BewareMyPower i totally agree with this and appreciate your feedback. For reference, my motivation in choosing the fourth method was because there is already a version.txt file that was included in the repo - which was being read by the pkg/mac/build-mac-wheels.sh script. I was trying to keep that component as similar as possible. With that said, I think this is overall better and more pythonic, and i've updated the bash script to continue to read in the version info (assuming the structure of the variable and its definition provided). I've pushed the latest commit, if you have a moment to take a look and make sure this looks good, it would be much appreciated. Thank you! |
This PR addresses #27 - Specifically, it does the following:
version.txt
file to within the pulsar module directory, which allows access both by top-level files (including setup.py, and the mac build bash scripts)init.py
inside the Python module to initialize a__version__
attribute at runtimeIn the end, this should hopefully maintain existing uses of the version file without breakage, but also provide for the requested version attribute that can be accessed in the standard way. (We could also look to using a dependency package like importlib, see the recommendations: https://peps.python.org/pep-0396/)