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

Remove incremental #491

Closed
wants to merge 4 commits into from
Closed

Conversation

SmileyChris
Copy link
Contributor

@SmileyChris SmileyChris commented Mar 27, 2023

Description

Remove Incremental as a requirement (but still support its use for project versions/names)

Fixes #308 and #398

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.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Patch coverage: 86.04% and project coverage change: -0.80 ⚠️

Comparison is base (70da86f) 99.84% compared to head (8667326) 99.05%.

❗ Current head 8667326 differs from pull request most recent head aa5e214. Consider uploading reports for the commit aa5e214 to get more accurate results

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     
Impacted Files Coverage Δ
src/towncrier/__init__.py 100.00% <ø> (ø)
src/towncrier/_shell.py 89.47% <66.66%> (-10.53%) ⬇️
src/towncrier/_settings/load.py 98.01% <84.61%> (-1.99%) ⬇️
src/towncrier/_project.py 95.74% <88.88%> (-4.26%) ⬇️
src/towncrier/build.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@SmileyChris SmileyChris force-pushed the remove-incremental branch 2 times, most recently from 27a8f71 to cc040d0 Compare May 2, 2023 02:03
@SmileyChris SmileyChris marked this pull request as ready for review May 2, 2023 03:37
@SmileyChris SmileyChris requested a review from a team as a code owner May 2, 2023 03:37
@adiroiban
Copy link
Member

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 incremental dependency from towncrier.
I think that managing it via a simple string is good enough.
The version should only be modified by the release manager and the release manager should be familiar with the versioning scheme.

I think that using Version('towncrier', 19, 9, 0, release_candidate=1) instead of just 19.9.0.rc1 is overengineering

@SmileyChris
Copy link
Contributor Author

@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 ;) )

Copy link
Member

@adiroiban adiroiban left a 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.

from .build import _main as _build_cmd
from .check import _main as _check_cmd
from .create import _main as _create_cmd


try:
Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor

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:
Copy link
Member

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.
Copy link
Member

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.

src/towncrier/test/test_project.py Show resolved Hide resolved
src/towncrier/test/test_project.py Show resolved Hide resolved
src/towncrier/test/test_project.py Show resolved Hide resolved
@adiroiban
Copy link
Member

@SmileyChris #482 it's on my todo list... but we can trigger a release anytime.

@hynek
Copy link
Member

hynek commented Jun 12, 2023

JFTR: if you'd like to entertain the idea of using hatch-vcs to deduplicate version information, I'm happy to help. I used to be skeptical to the idea, but now I've moved all my projects to it. If configured correctly, it also allows for continuous uploads to Test PyPI: https://test.pypi.org/project/structlog/#history

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.

@adiroiban
Copy link
Member

To me hatch-vcs is over-engineering.
I am not sure what problem it tries to solve.

Is it that hard to keep the version as a text inside a .toml file? :)

@hynek
Copy link
Member

hynek commented Jun 12, 2023

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. :)

@hynek
Copy link
Member

hynek commented Jun 23, 2023

this needs a rebase / conflict resolution

@SmileyChris SmileyChris force-pushed the remove-incremental branch 2 times, most recently from 26c694c to 7c7a930 Compare June 23, 2023 12:18
@SmileyChris
Copy link
Contributor Author

SmileyChris commented Jun 23, 2023

Removed the python 3.7 related fallbacks, updated the version in the toml, and rebased against trunk.
(and pushing again with no change because the checknewsfragment test failed for no good reason 😖 )

Copy link
Member

@hynek hynek left a 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. ;)

Comment on lines -16 to -32
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__
Copy link
Member

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 ;))

Copy link
Contributor

Choose a reason for hiding this comment

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

re-added #627

@@ -38,47 +38,45 @@ def _get_package(package_dir: str, package: str) -> ModuleType:


def get_version(package_dir: str, package: str) -> str:
module = _get_package(package_dir, package)
version: Any
Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

changed to str #627

Comment on lines +77 to +80
try:
return str(version.package) # type: ignore
except AttributeError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor

Choose a reason for hiding this comment

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

done #627

from .build import _main as _build_cmd
from .check import _main as _check_cmd
from .create import _main as _create_cmd


try:
Copy link
Member

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):
Copy link
Member

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.

Copy link
Contributor

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:
Copy link
Member

@adiroiban adiroiban Jul 9, 2023

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

Copy link
Contributor

@sigma67 sigma67 Jul 28, 2024

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

@adiroiban adiroiban mentioned this pull request Nov 7, 2023
4 tasks
@sigma67
Copy link
Contributor

sigma67 commented Jul 26, 2024

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
Copy link
Contributor

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.

Copy link
Contributor

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?

@adiroiban
Copy link
Member

Hi @sigma67

This PR is stale.

If you have time, feel free to continue working on it.

Thanks

@sigma67
Copy link
Contributor

sigma67 commented Jul 28, 2024

Hi @adiroiban, thanks, I've picked it up in #627

@adiroiban
Copy link
Member

I am closing this as the current effort is with #627

@adiroiban adiroiban closed this Jul 29, 2024
@sigma67 sigma67 mentioned this pull request Jul 29, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on incremental in favor of general compatible solution
6 participants