search

trinodb / trino-python-client

Python client for Trino
Apache License 2.0
307 stars 151 forks source link

separate __version__ from __init__ #410

Closed aksakalli closed 9 months ago

aksakalli commented 9 months ago

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 commented 9 months ago

Can we merge this? @hashhar @hovaesco

aksakalli commented 9 months ago

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.

https://github.com/trinodb/trino-python-client/blob/d6dee794eeecf59ab5725671094ec7e5aa741fff/.github/workflows/release.yml#L30

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

hashhar commented 9 months ago

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.

aksakalli commented 9 months ago

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 commented 9 months ago

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.