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

Show not-yet-released change notes in Sphinx docs #2650

Merged
merged 4 commits into from May 9, 2021

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Apr 21, 2021

Summary of changes

The recent towncrier release now requires a {{ top_line }} variable in the template (as discovered in pypa/pip#9815). Without this change, the changelog updates will have no title when the new Towncrier is used and this will happen sooner or later because it's unpinned in the deps.

Also, besides fixing that problem above this change integrates an extension for showing the upcoming release change notes in the Sphinx docs whenever standalone fragments are present in the tree.

Blocked by: #2651.

Pull Request Checklist

Ref: pypa/pip#9815

webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 21, 2021
@webknjaz webknjaz changed the title Make changelog tmpl compatible w/ new Towncrier Show not-yet-released change notes in Sphinx docs Apr 21, 2021
@webknjaz webknjaz requested a review from jaraco April 21, 2021 17:02
docs/conf.py Outdated
Comment on lines 115 to 126
PROJECT_ROOT_DIR = Path(__file__).parents[1].resolve()
get_scm_version = partial(get_version, root=PROJECT_ROOT_DIR)

# The short X.Y version
version = '.'.join(
get_scm_version(
local_scheme='no-local-version',
).split('.')[:3],
)

# The full version, including alpha/beta/rc tags
release = get_scm_version()
Copy link
Member

Choose a reason for hiding this comment

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

This logic is redundant and conflicts with the work done by jaraco.packaging.sphinx. Is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's needed because otherwise jaraco.packaging.sphinx selects the hardcoded version of setuptools which is something that's already been released while this match makes it the next guess provided by setuptools-scm.

If I remove this, the changelog would look like:

# v56
- entries that are still in the form of fragments
# v56
- entries you've generated on release

but the goal of this change is to have something like

# v57.dev0
- entries that are still in the form of fragments
# v56
- entries you've generated on release

So that next version preview should either be generated here in sphinx or you should modify your release flow to change the hardcoded version to v57.dev0 right after the release.

Here's what it looks like:
setuptools-towncrier-draft

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I would actually like to get this project enabled with setuptools_scm, but until the project itself can adopt setuptools_scm, it seems a bad idea to paper over the issue that setup.py --version is reporting an incorrect version by partially adopting setuptools_scm but only in docs.

Currently, setuptools defines egg_info.tag_build and egg_info.tag_date to provide a post-release tag. I'm a little surprised that setup.py --version (what jaraco.packaging currently uses to elicit the version) doesn't include that tag. That's probably another bug.

Regardless, jaraco.packaging should probably invoke PEP 517 to get the package metadata and the version from that. Doing so would have the unfortunate side-effect of rendering the .post and date even for tagged commits (which are only disabled when cutting a release).

Since adopting setuptools_scm for this project would solve all of these problems, I'd like to first tackle that issue (#2656).

Copy link
Member Author

@webknjaz webknjaz Apr 26, 2021

Choose a reason for hiding this comment

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

Alright, I'll change the approach a bit and will just remove the version from this title, for the time being, it's not that important. We can re-add it later, once #2656 is in.

@jaraco
Copy link
Member

jaraco commented Apr 24, 2021

Can you please provide an explanation of what the issue is? Where does towncrier make top_line required? I don't see mention of it in the docs. It feels like this PR is trying to solve two issues. Maybe consider separating the two?

@webknjaz
Copy link
Member Author

Can you please provide an explanation of what the issue is? Where does towncrier make top_line required? I don't see mention of it in the docs. It feels like this PR is trying to solve two issues. Maybe consider separating the two?

Yes, you are right. I'll make a separate PR. Towncrier's own changelog is not very explicit about this but the old version (before 19.9) used to prepend the title outside the template and starting 19.9+ it just relies for the variable to be present inside of it.

webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 25, 2021
@webknjaz
Copy link
Member Author

I've filed that patch as #2654 but also kept the corresponding commit in this branch as a base.

webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 25, 2021
webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 25, 2021
webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 25, 2021
@webknjaz
Copy link
Member Author

@jaraco looks like we've found a side-effect of having non-integrated packaging-related deps in envs with tests... Any ideas?

webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 25, 2021
webknjaz added a commit to webknjaz/setuptools that referenced this pull request Apr 26, 2021
@webknjaz webknjaz force-pushed the docs/towncrier-upgrade branch 2 times, most recently from 464aa59 to 3329a7e Compare April 26, 2021 12:26
@webknjaz webknjaz requested a review from jaraco April 26, 2021 12:26
@webknjaz
Copy link
Member Author

webknjaz commented Apr 26, 2021

@jaraco I dropped setuptools-scm from this PR, it should be good to go now: https://setuptools--2650.org.readthedocs.build/en/2650/history.html.

@webknjaz
Copy link
Member Author

webknjaz commented May 4, 2021

@jaraco I rebased this to fix the conflicts.

docs/conf.py Outdated
Comment on lines 154 to 161
# -- Options for towncrier_draft extension --------------------------------------------

PROJECT_ROOT_DIR = Path(__file__).parents[1].resolve()

towncrier_draft_autoversion_mode = "draft" # or: 'sphinx-release', 'sphinx-version'
towncrier_draft_include_empty = True
towncrier_draft_working_directory = PROJECT_ROOT_DIR
# Not yet supported: towncrier_draft_config_path = 'pyproject.toml' # relative to cwd
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 annoyed that towncrier_draft requires all of these settings. Why aren't the defaults good enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just didn't want to assume the working directory because cwd may depend on what you set in changedir in tox.ini effectively making this setup extremely fragile when the setting there is changed because the connection is highly unobvious. I guess I could attempt implementing some sort of an autodetection on the extension side but for now, it only implements a bare minimum w/o making assumptions.

Essentially, if I were to look into the config and see .., I'd assume it's relative to the dir with conf.py but it's relative to the process' current working directory.

@jaraco
Copy link
Member

jaraco commented May 9, 2021

I'm a little meh on this change. Setuptools aims to do frequent releases, so the benefit of generating changelog entries for yet unreleased changes is of little value for that uncommon scenario. I'm not necessarily opposed to it as long as it doesn't add a lot of debt to the project. The fact that it has to hard-code the path to the project and specify a whole selection of settings feels a little cumbersome to me. I'd be much more inclined to accept this change if enabling the extension and adding the directive was all that was needed.

@jaraco
Copy link
Member

jaraco commented May 9, 2021

This latest change is much cleaner. I can live with this... as long as it looks okay during a release.

@jaraco jaraco merged commit 99cabc2 into pypa:main May 9, 2021
@jaraco
Copy link
Member

jaraco commented May 9, 2021

I'm sad to report that towncrier_draft still generates the content even when there's nothing to report, so here's what the official release docs look like:

image

That's ugly. I'm going to undo the merge.

@jaraco
Copy link
Member

jaraco commented May 9, 2021

I've force pushed 317ad2c to main, effectively removing the merge commit 99cabc2, and opening up the prospect of re-submitting fef727f as a new PR, but honestly, I wouldn't bother. The value proposition is small and the trouble seems to be non-trivial. Still, I won't prescribe. I'll accept a clean, concise application of this technique that doesn't clutter the official docs.

@webknjaz
Copy link
Member Author

webknjaz commented May 9, 2021

I believe with towncrier_draft_include_empty = False, it'd avoid showing that section when it's unnecessary, IIRC.

@jaraco
Copy link
Member

jaraco commented May 19, 2021

I believe with towncrier_draft_include_empty = False, it'd avoid showing that section when it's unnecessary, IIRC.

I confirmed this is the case. Sure would be nice if this behavior were the default. I'll be submitting a new PR shortly.

@webknjaz
Copy link
Member Author

Sure would be nice if this behavior were the default

IIRC we've hit some corner case with this in the pip docs: pypa/pip#9505.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants