-
Notifications
You must be signed in to change notification settings - Fork 128
Remove incremental #491
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
Remove incremental #491
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## trunk #491 +/- ##
==========================================
- Coverage 99.84% 99.05% -0.80%
==========================================
Files 13 12 -1
Lines 631 632 +1
Branches 146 143 -3
==========================================
- Hits 630 626 -4
- Misses 0 4 +4
- Partials 1 2 +1
☔ View full report in Codecov by Sentry. |
27a8f71 to
cc040d0
Compare
d1f829a to
ad93b60
Compare
|
Thanks, Chris for working on this. I am working to make a new release of towncrier, and the plan is to drop 3.7 support. So this PR should be even simpler. I am happy to drop the I think that using |
|
@adiroiban cool! I'd say merge this first then work on dropping the 3.7 support, otherwise I can update this after you've changed the automated tests so this doesn't fail when I remove the import try/excepts. (also a review of #482 would be nice, it would be a good addition before the new release ;) ) |
adiroiban
left a comment
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.
The changes look good.
Only minor comments.
I think that it would be nice to be able to still do from towncrier._version import version or something similar, without having to worry about importlib each time.
Now, I don't know if it should raise an exception or return a dummy string, when the version was not found... I would go with a "dummy/placeholder" string.
src/towncrier/_shell.py
Outdated
| from .create import _main as _create_cmd | ||
|
|
||
|
|
||
| try: |
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.
Maybe we can still keep the _version.py file but this time just just a _version string variable.
I this way, we don't have to repeat the importlib code each time we want to fetch the version
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.
We can do this lazily using towncrier.__version__ if the block that I've commented on earlier is ported and kill two birds with one stone.
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.
_version.py restored with contents
__version__ = "23.11.1.dev0"
| from importlib_metadata import PackageNotFoundError, version # type: ignore | ||
|
|
||
|
|
||
| try: |
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.
this code can be included in the updated _version.py code
| """ | ||
| Import __version__ from towncrier returns an Incremental version object | ||
| and raises a warning. | ||
| towncrier.__version__ was deprecated, now no longer exists. |
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.
not sure if towncrier.__version__ was considered public or private API.
For private API, raising AttributeError is ok.
If it's public API, we need to raise a separate error with an info message.
|
@SmileyChris #482 it's on my todo list... but we can trigger a release anytime. |
|
JFTR: if you'd like to entertain the idea of using I guess the "you" here is mostly towards @adiroiban since he's doing all the release work and @glyph because he's The Twisted King. |
|
To me Is it that hard to keep the version as a text inside a .toml file? :) |
|
Well, it's not. :) But it deduplicates the version information to one place (we do git tags too) and enables the continuous uploads I've mentioned. But obviously, if you don't want it, it's not happening. Just wanted to offer. :) |
|
this needs a rebase / conflict resolution |
26c694c to
7c7a930
Compare
|
Removed the python 3.7 related fallbacks, updated the version in the toml, and rebased against trunk. |
7c7a930 to
e0df0a7
Compare
e0df0a7 to
396a84b
Compare
hynek
left a comment
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.
I've added a few notes, but ultimately I'm unconditionally yielding the decisions to @adiroiban since this has been his dream for years. ;)
| def __getattr__(name: str) -> Version: | ||
| if name != "__version__": | ||
| raise AttributeError(f"module {__name__} has no attribute {name}") | ||
|
|
||
| import warnings | ||
|
|
||
| from ._version import __version__ | ||
|
|
||
| warnings.warn( | ||
| "Accessing towncrier.__version__ is deprecated and will be " | ||
| "removed in a future release. Use importlib.metadata directly " | ||
| "to query for towncrier's packaging metadata.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
|
||
| return __version__ |
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.
I'm not sure this should be removed. It seems like there's still a bit of software relying on pkg.__version__ such that I've starte de-deprecating it in my own packages. See also python-attrs/attrs#1136
(to be clear: it has to be ported, not just left around ;))
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.
re-added #627
|
|
||
| def get_version(package_dir: str, package: str) -> str: | ||
| module = _get_package(package_dir, package) | ||
| version: Any |
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.
Why Any? Any opts version completely out of type checking, is there a rescue for that?
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.
changed to str #627
| try: | ||
| return str(version.package) # type: ignore | ||
| except AttributeError: | ||
| pass |
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.
| try: | |
| return str(version.package) # type: ignore | |
| except AttributeError: | |
| pass | |
| with contextlib.suppress(AttributeError): | |
| return str(version.package) # type: ignore |
is more idiomatic, I think
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.
done #627
src/towncrier/_shell.py
Outdated
| from .create import _main as _create_cmd | ||
|
|
||
|
|
||
| try: |
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.
We can do this lazily using towncrier.__version__ if the block that I've commented on earlier is ported and kill two birds with one stone.
|
|
||
| class TestPackaging(TestCase): | ||
| def test_version_warning(self): | ||
| def no_version_attr(self): |
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.
I don't think this runs if it doesn't have a test_ prefix.
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.
changed test to check for deprecationwarning instead #627
|
|
||
| if not version: | ||
| raise Exception("No __version__, I don't know how else to look") | ||
| try: |
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 looks like this is not only about removing incremental, but also about using importlib_metadata to get the version of the target project
I think is best to do this as part of a separate PR ..l like #502
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.
Stale comment as linked PR has been merged. Updated and integrated latest trunk in #627
|
Hi @adiroiban @SmileyChris is this PR stale and ready to be picked up by someone else? Or are you planning to continue work? |
| version = str(version.base()).strip() | ||
| # Incremental uses `X.Y.rcN`. | ||
| # Standardize on importlib (and PEP440) use of `X.YrcN`: | ||
| return version.replace(".rc", "rc") # type: ignore |
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.
FYI this has been fixed on the Incremental side.
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.
Not sure what you are referring to. Should anything in this PR be changed as a result?
|
Hi @sigma67 This PR is stale. If you have time, feel free to continue working on it. Thanks |
|
Hi @adiroiban, thanks, I've picked it up in #627 |
|
I am closing this as the current effort is with #627 |
Description
Remove Incremental as a requirement (but still support its use for project versions/names)
Fixes #308 and #398
Checklist
src/towncrier/newsfragments/. Describe yourchange and include important information. Your change will be included in the public release notes.
docs/tutorial.rstis still up-to-date.docs/cli.rstreflects those changes.docs/configuration.rstreflects those changes.