♻️ Remove incremental by sigma67 · Pull Request #627 · twisted/towncrier

sigma67 · 2024-07-28T09:33:40Z

Description

Fixes #308

This removes the dependency on incremental on towncrier itself, while keeping support for detecting incremental version in projects for which towncrier is used to build the release notes.

Checklist

Make sure changes are covered by existing or new tests.
For at least one Python version, make sure local test run is green.
Create a file in src/towncrier/newsfragments/. Describe your
change and include important information. Your change will be included in the public release notes.
Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
Ensure docs/tutorial.rst is still up-to-date.
If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

adiroiban

Many thanks for picking up on this.

I just did a quick review before approving the CI run.

I have not yet fully reviewed the automated tests.

adiroiban · 2024-07-28T10:37:57Z

RELEASE.rst


 Update the version to the release candidate with the first being ``rc1`` (as opposed to 0).
-In ``src/towncrier/_version.py`` the version is set using ``incremental`` such as::
+In ``pyproject.toml`` the version is set like::


I was hoping that we can set the version in single place, like src/towncrier/_version.py and the build system can automatically extract the version from there.

The changes in doc were recycled but now they are no longer relevant. Reverted

adiroiban · 2024-07-28T10:40:37Z

docs/conf.py

 # Add any Sphinx extension module names here, as strings. They can be
 # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
 # ones.
+import importlib.metadata as importlib_metadata


Why not just use from towncrier import __verison__ or something similar ?

The changes in doc were recycled but now they are no longer relevant. Reverted

adiroiban · 2024-07-28T10:41:17Z

src/towncrier/_project.py

-        raise Exception(
-            f"No __version__ or metadata version info for the '{package}' package."
-        )
+        try:


I this change needed ?

I was thinking that this was sorted out in #502

Right, I think it's just a merge artifact. Reverted

adiroiban · 2024-07-28T10:43:48Z

src/towncrier/_shell.py

+
 import click

-from ._version import __version__


Why not keep the import ?

It's just that now __version__ will be a simple text.

adiroiban · 2024-07-29T08:23:26Z

src/towncrier/test/test_packaging.py


 class TestPackaging(TestCase):
-    def test_version_warning(self):
+    def test_version_attr(self):


I am not sure why this change is needed.

The previous test look good enough to me. :)

I am misssing something ?

The previous test was checking for the presence of an Incremental Version object (which obviously no longer exists in towncrier)

I've replaced it with a test to assert that a DeprecationWarning is present

src/towncrier/test/test_project.py

adiroiban

Thanks for the changes.
They look good.

I left a few minor comments.

In order to merge this, the mypy checks must pass.

Also the coverage must pass

src/towncrier/_project.py      52      1     18      1    97%   90, 104->exit

adiroiban · 2024-07-29T08:30:18Z

src/towncrier/_project.py

-
    version = getattr(module, "__version__", None)
+    # Incremental has support for package names, try duck-typing it.
+    with contextlib.suppress(AttributeError):


I think that we are missing a brach test here

I still see this error reported

line 104 didn't return from function 'get_project_name' because the return on line 105 wasn't executed

The miss indicator was a bit confusing for me, should be gone now hopefully

adiroiban · 2024-07-29T08:30:45Z

src/towncrier/_project.py

+            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


I don't know why coverage is reporting this line as not covered.... we need to check

There was an indentation problem with a test, fixed now

sigma67

thanks, mypy and coverage should be passing now

sigma67 · 2024-07-29T10:34:48Z

src/towncrier/_project.py

-
    version = getattr(module, "__version__", None)
+    # Incremental has support for package names, try duck-typing it.
+    with contextlib.suppress(AttributeError):


sigma67 · 2024-07-29T10:37:24Z

src/towncrier/test/test_packaging.py


 class TestPackaging(TestCase):
-    def test_version_warning(self):
+    def test_version_attr(self):


The previous test was checking for the presence of an Incremental Version object (which obviously no longer exists in towncrier)

I've replaced it with a test to assert that a DeprecationWarning is present

sigma67 · 2024-07-29T10:37:51Z

src/towncrier/_project.py

+            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 an indentation problem with a test, fixed now

sigma67 · 2024-07-29T13:23:31Z

@adiroiban just to know a timeline, is there any ETA on the next release that could include this change?

This is just a question, no pressure

adiroiban · 2024-07-29T13:32:34Z

We can do a release.
There is already a ticket for that here #616

the important part is to have someone who cares for a new release and has some time to test the release

adiroiban · 2024-07-29T14:06:02Z

Thanks for fixing all the checks.

As part of this PR we need to update the RELEASE.rst ... if we just merge, the information for RELEASE.rst will no longer be valid

I now see that RELEASE.rst changes were removed from this PR

https://github.com/twisted/towncrier/pull/491/files#diff-0a92214d83ea13ad864f4b1d6c0b5703956cf4bd927c116e75f68d4048e0744f

My hope was that with this PR we can have pyproject.toml with

[metadata]
version = attr: towncrier.__version__

adiroiban · 2024-07-29T14:36:20Z

I made a few small changes to the PR description... the important one is linking to the issue, so that when we merge this PR, the issue is automatically closed.

sigma67 · 2024-07-29T15:40:57Z

Ok, I'm a bit confused admittedly because in the prior MR it was stated explicitly to keep the version file.

Here: #491 (comment)

If we still want to move to pyproject.toml I'd prefer to do that in a separate MR.

What do you want changed in RELEASE.rst ? Should I update the version as well or is that done by the release manager?

adiroiban · 2024-07-29T18:03:22Z

Right now the content of the RELEASE.rst is

towncrier/RELEASE.rst

Lines 21 to 24 in 4b58be5

    
           Update the version to the release candidate with the first being ``rc1`` (as opposed to 0). 
        
           In ``src/towncrier/_version.py`` the version is set using ``incremental`` such as:: 
        
               __version__ = Version('towncrier', 19, 9, 0, release_candidate=1)

I think it should change to

 Update the version to the release candidate with the first being ``rc1`` (as opposed to 0). 
 In ``src/towncrier/_version.py`` the version is set using PEP440 a compliant string, such as:: 
  
     __version__ = "24.7.0rc1"

This revert is not quite OK 2cf4e76

Instead of updating the pyproject.toml we update the _version.py file.

We no longer want to use incremental . This is why towncrier own development documentation should no longer instruct developers to use incremental

But I understand your concern.

The question is where to store the version:

pyproject.toml
src/towncrier/_version.py
src/towncier/__init__.py

Since the version was already stored in src/towncrier/_version.py , my suggestion for this PR is to continue to store it there.

In a separate PR we can consider moving it inside pyproject.toml or somewhere else.

sigma67 · 2024-07-29T19:50:05Z

Ok my bad, I guess it only needed some changes instead of a revert. RELEASE.rst is updated.

adiroiban

Many thanks for the update.

I did a few change to release.rst

There was a section underline mismatch ... so I start doing the edit and then I realized that 19.9.0 is a better example as it shows that we don't add leasing 0 in the version ... but we use ISO date format with leading zero.

adiroiban · 2024-07-30T10:40:21Z

I have enabled auto-merge on this.

and once this is merge I plan to do a new release.

Thanks again.

I am very happy to see that we no longer use incremental.

SmileyChris added 3 commits June 24, 2023 00:28

Remove incremental

90656a1

Add news fragment

396a84b

Update tutorial for accuracy around detecting versions

accc883

sigma67 requested a review from a team as a code owner July 28, 2024 09:33

Merge branch 'trunk' into remove-incremental

dd20edf

sigma67 force-pushed the remove-incremental branch from a037030 to dd20edf Compare July 28, 2024 09:51

sigma67 added 2 commits July 28, 2024 11:55

Merge remote-tracking branch 'origin/trunk' into remove-incremental

08de244

review findings and fixes

cf2df80

sigma67 mentioned this pull request Jul 28, 2024

Remove incremental #491

Closed

7 tasks

adiroiban reviewed Jul 28, 2024

View reviewed changes

sigma67 added 3 commits July 28, 2024 21:55

review findings

198ad35

revert RELEASE.rst

2cf4e76

fix rtd build

57022cc

adiroiban reviewed Jul 29, 2024

View reviewed changes

src/towncrier/test/test_project.py Outdated Show resolved Hide resolved

adiroiban requested changes Jul 29, 2024

View reviewed changes

adiroiban reviewed Jul 29, 2024

View reviewed changes

sigma67 and others added 2 commits July 29, 2024 11:11

Update src/towncrier/test/test_project.py

7e9d4f0

fix coverage gaps

03894fe

sigma67 commented Jul 29, 2024

View reviewed changes

fix coverage gap

1f35abd

adiroiban mentioned this pull request Jul 29, 2024

Check if any news fragments have invalid filenames #622

Merged

7 tasks

update RELEASE.rst

d96226a

Use 19.9.0 as an example to demonstrate how zero padding is used

c23eade

adiroiban approved these changes Jul 30, 2024

View reviewed changes

Merge branch 'trunk' into remove-incremental

2e059fa

adiroiban enabled auto-merge (squash) July 30, 2024 10:40

adiroiban merged commit 0fa7b48 into twisted:trunk Jul 30, 2024

Conversation

sigma67 commented Jul 28, 2024 • edited by adiroiban Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adiroiban Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigma67 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigma67 commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adiroiban commented Jul 29, 2024

Uh oh!

adiroiban commented Jul 29, 2024

Uh oh!

adiroiban commented Jul 29, 2024

Uh oh!

sigma67 commented Jul 29, 2024

Uh oh!

adiroiban commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sigma67 commented Jul 29, 2024

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

adiroiban commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

sigma67 commented Jul 28, 2024 •

edited by adiroiban

Loading

adiroiban Jul 29, 2024 •

edited

Loading

sigma67 commented Jul 29, 2024 •

edited

Loading

adiroiban commented Jul 29, 2024 •

edited

Loading