Skip to content

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

Merged
merged 6 commits into from
Jan 13, 2023
Merged

Add a version attribute #81

merged 6 commits into from
Jan 13, 2023

Conversation

erichare
Copy link
Contributor

@erichare erichare commented Jan 5, 2023

This PR addresses #27 - Specifically, it does the following:

  • Moves the 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)
  • Updates the existing references to the version.txt file to ensure that the sub-directory is included in the full path
  • Adds to init.py inside the Python module to initialize a __version__ attribute at runtime

In 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/)

@eolivelli
Copy link

Can you lease describe the modifications?

@erichare
Copy link
Contributor Author

erichare commented Jan 5, 2023

Can you lease describe the modifications?

@eolivelli thanks, i just updated the description in the parent comment to describe what is being done!

Copy link
Member

@michaeljmarshall michaeljmarshall left a 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.

@@ -62,6 +62,11 @@
from datetime import timedelta


# Get the pulsar version from version.txt
with open("version.txt") as f:
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @tisonkun . Do you mind taking a quick look at: da872e0 - is this the right approach (similar to what is done in the setup.py file)

Copy link
Contributor

@BewareMyPower BewareMyPower Jan 10, 2023

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

@BewareMyPower BewareMyPower changed the title #27 Add a version attribute Add a version attribute Jan 10, 2023
@BewareMyPower
Copy link
Contributor

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:

With this approach you must make sure that the VERSION file is included in all your source and binary distributions

The version.txt should be packaged into the Python wheel. I'm not familiar with the PyPI setup.py and you might need to search for how to package it.

If I were you, I would adopt the 3rd method, which is simple and clear.

Create an __about__.py, which defines a __version__ global variable, in the pulsar/ directory:

__version__='3.1.0a1'

Then, in pulsar/__init__.py you can get the version by:

from pulsar.__about__ import __version__

And in setup.py you can change the get_version function to:

def get_version():
    version = {}
    with open("./pulsar/__about__.py") as fp:
        exec(fp.read(), version)
    return version['__version__']

References:

@erichare
Copy link
Contributor Author

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:

With this approach you must make sure that the VERSION file is included in all your source and binary distributions

The version.txt should be packaged into the Python wheel. I'm not familiar with the PyPI setup.py and you might need to search for how to package it.

If I were you, I would adopt the 3rd method, which is simple and clear.

Create an __about__.py, which defines a __version__ global variable, in the pulsar/ directory:

__version__='3.1.0a1'

Then, in pulsar/__init__.py you can get the version by:

from pulsar.__about__ import __version__

And in setup.py you can change the get_version function to:

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!

@erichare erichare requested review from BewareMyPower and removed request for merlimat January 12, 2023 15:43
@BewareMyPower BewareMyPower merged commit fe6dc94 into apache:main Jan 13, 2023
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.

5 participants