-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat: Read plugin version from distribution, with fallback to package #1254
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
- Coverage 83.85% 83.82% -0.03%
==========================================
Files 44 44
Lines 4150 4155 +5
Branches 718 719 +1
==========================================
+ Hits 3480 3483 +3
- Misses 490 491 +1
- Partials 180 181 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Windows tests are failing, and I don't have an environment (or the familiarity of Windows+Python 😅) to debug. Given this doesn't cause a hard-fail (we simply get |
if platform.system() == "Windows": | ||
pytest.xfail("Doesn't pass on windows.") |
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.
Does this mean the package name can't be retrieved in Windows?
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.
It means the lookup via distribution will not work, yes. The fallback to metadata.version(cls.name)
was the primary mechanism before this change (it relies on the library name being the same as the class name), though I don't know how well it worked for Windows in the first place 🤔
Given the hurdles we have to jump through to try and infer the plugin version via distribution or library metadata, I still think we are better off adding __version__ = "<version>"
in __init__.py
and importing that for --about
. poetry-dynamic-versioning
supports substitution from a 0.0.0
placeholder in any file (including __init__.py
by default), and this approach is standard in other python packages 🤷♂️ This PR specifically addresses this ask from @aaronsteers 👍
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.
@kgpayne Yeah, I'd say it's probably not worth spending time trying to solve a problem that's only present when migrating existing taps and targets that are published to PyPI. Wdyt?
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.
approved, pending that one open thread, but looks good to me
Closing in favour of
|
In cases where the distribution name does not match the tap/target ID (also the package name by default) the
Version
reported in--about
registers as[could not be detected]
.e.g.
tap-snowflake
with package nametap_snowflake
distributed asmeltanolabs-tap-snowflake
, to avoid PyPI name clashes.Related to MeltanoLabs/tap-snowflake#8
📚 Documentation preview 📚: https://meltano-sdk--1254.org.readthedocs.build/en/1254/