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

separate __version__ from __init__ #410

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

aksakalli
Copy link
Member

@aksakalli aksakalli commented Sep 15, 2023

Description

As part of adding a user-agent to client feature #406, trino.__version__ should be accessible within the package itself otherwise a statement such as

from trino import __version__

would cause a circular dependency. Everything within a package should be able to be imported correctly before importing the root level __init__.py.

Having a "private" module trino._version to store meta info about this package is a pseudo convention that followed by many popular python libraries. GitHub contains 18.8k repos with such convention according to this search.

For more explanation:

Non-technical explanation

This change will make the version information trino.__version__ accessible from a central place within the package itself.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

@aksakalli
Copy link
Member Author

Can we merge this? @hashhar @hovaesco

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Good to go but I need to adjust https://github.com/trinodb/trino-python-client/blob/master/.github/workflows/release.yml to this.

That's why I've not merged it yet.

@aksakalli
Copy link
Member Author

Good to go but I need to adjust https://github.com/trinodb/trino-python-client/blob/master/.github/workflows/release.yml to this.

That's why I've not merged it yet.

Sorry, I missed that out. I didn't know that the next version is generated from CI.

sed -i "s/${VERSION_NUMBER}/${NEXT_VERSION_NUMBER}/g" trino/__init__.py

I can simply replace trino/__init__.py with trino/_version.py?

__version__ is defined in __init__.py in trino module which means that
we cannot easily use the module version in the library code because it
introduces a cyclic dependency.

This change introduces a _version.py which also includes other
meta-information and changes setup.py to read those values from there
instead of inline.

The release script is also updated since the version now needs to be
updated in a different file.
@hashhar
Copy link
Member

hashhar commented Sep 20, 2023

I've just reworded the commit message + squashed (otherwise there would be one commit at which point automated releases wouldn't work).

Will merge once CI is finished.

@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Sep 20, 2023
@aksakalli
Copy link
Member Author

aksakalli commented Sep 20, 2023

I've just reworded the commit message + squashed (otherwise there would be one commit at which point automated releases wouldn't work).

Will merge once CI is finished.

I don't think force push to a feature branch is the best approach, you can always squash merge to avoid that happening. That's the convention most repos are following including trindb/trino actually not Trino :) I don't know the reasoning exactly.

@hashhar
Copy link
Member

hashhar commented Sep 20, 2023

squash merge to avoid that happening.

Needs to be enabled per repo. We don't have it enabled on this. It's enabled on trinodb/trino but don't always squash merge since in most cases the history and smaller commits are useful.

@hashhar hashhar merged commit 31febef into trinodb:master Sep 20, 2023
12 checks passed
@aksakalli aksakalli deleted the refactor-setup branch September 20, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants