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

Add version as an importable variable and remove dependency on pkg_resources #566

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Add version as an importable variable and remove dependency on pkg_resources #566

merged 1 commit into from
Jun 9, 2020

Conversation

sjhewitt
Copy link
Contributor

@sjhewitt sjhewitt commented Jun 6, 2020

What does this PR do?

Remove the dependency on pkg_resources by moving the version to datadog/version.py

Description of the Change

importing of pkg_resources can be slow as it does a lot of import-time processing (see pypa/setuptools#926, pypa/setuptools#510)

This change moves the version from setup.py into a file inside the datadog module so that it can be imported from other locations in the code (most notably it can be imported from the dogstatsd module without causing a circular import)

Alternate Designs

This document outlines the possible approaches: https://packaging.python.org/guides/single-sourcing-package-version/

  • Option 1 is not possible as the version needs to be accessible from the dogstatsd module and this approach would cause a circular import
  • Option 2 would cause bigger changes to the release process
  • Option 3 is the chosen option
  • Option 4 requires changes to the manifest, and would require extra functions to load the version in other places
  • Option 5 is available to python3.8 only, and falls back to pkg_resources
  • Option 6 is not recommended, but is commonly used
  • Option 7 requires more dependencies and configuration

Possible Drawbacks

The main drawback is that it is a change to the release process, which may cause a small amount of friction when releasing a new version.

Verification Process

  • I have used setup.py and pip to install the library
  • successfully run python setup.py --version

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@sjhewitt sjhewitt requested a review from a team as a code owner June 6, 2020 06:47
Copy link
Contributor

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR !
Just one comment, LGTM otherwise.

Comment on lines 135 to 160
def get_pkg_version():
"""
Resolve `datadog` package version.
"""
if not pkg:
return u"unknown"

dist = pkg.get_distribution("datadog")
# Normalize case for Windows systems
dist_loc = os.path.normcase(dist.location)
here = os.path.normcase(__file__)
if not here.startswith(dist_loc):
# not installed, but there is another version that *is*
raise pkg.DistributionNotFound

return dist.version


def get_version():
"""
Resolve `datadog` package version.
"""
try:
return get_pkg_version()
except pkg.DistributionNotFound:
return u"Please install `datadog` with setup.py"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep these two functions for compatibility, in case some people use it in their code.
Just make them return the __version__ variable, and maybe add a comment in the docstring we're keeping them for compatibility but user should import __version__ from version instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@zippolyte zippolyte added the changelog/Changed Changed features results into a major version bump label Jun 9, 2020
@zippolyte
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zippolyte zippolyte merged commit 058114c into DataDog:master Jun 9, 2020
@sjhewitt sjhewitt deleted the sjh-version-file branch June 9, 2020 15:54
@zippolyte zippolyte added changelog/Added Added features results into a minor version bump and removed changelog/Changed Changed features results into a major version bump labels Jun 9, 2020
@zippolyte zippolyte changed the title move version to importable file, remove pkg_resources import Add version as an importable variable and remove dependency on pkg_resources Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants