Make an illustrative change notes guide in GH UI by webknjaz · Pull Request #2395 · pypa/setuptools

webknjaz · 2020-09-20T19:23:43Z

Summary of changes

This change places a README.rst document under changelog.d/ dir in
order for GitHub to render it when users navigate to this location via
the web UI. So that PR submitters can navigate to changelog.d/ from
the main repo view and see something like this:
https://github.com/webknjaz/setuptools/tree/docs/changelog-fragments/changelog.d

It also includes that into the dev guide in Sphinx docs.

As a related change (totally atomic though), this PR also introduces a
:user: role that can be used in change note signatures as a means to
credit contributions to their authors.
(Demo: https://ansible-pylibssh.rtfd.io/en/latest/changelog/#vrelease-unreleased-draft-2020-09-09)

I've picked up this trick from tox and it's been working great for me so far.
Let me know what you think!

Pull Request Checklist

Changes have tests
News fragment added in changelog.d. See documentation for details

webknjaz · 2020-09-20T19:29:50Z

-norecursedirs=dist build *.egg setuptools/extern pkg_resources/extern pkg_resources/tests/data tools .* setuptools/_vendor pkg_resources/_vendor
+norecursedirs =
+    dist
+    docs


Apparently, pytest attempts to import Python modules from the docs dir too. I'm pretty sure this shouldn't be happening.

The --doctest-modules means that by default pytest will discover any and all Python modules and test the docs strings in them. This approach has the advantage that all Python modules get at least syntax checking. This behavior is employed across the projects I maintain and it helps catch errors early. It's a feature, not a bug. Please revert.

But the only Python module there is conf.py and it doesn't have any doctests inside. This was necessary because I used f-stings and it makes pytest fail altogether in all old envs.
Do you really want conf.py to be compatible with Python 2.7?

If you're concerned about the syntax checking, it wouldn't get worse since Travis CI always runs Sphinx and Netlify integration runs per PR too. (Did you know you can now enable RTD integration with PRs too?)

Everything that executes Sphinx config seems to be running Python 3.8. So do you still want this reverted? I don't see any compatibility guarantees for docs documented...

I just saw 896ef66. Would it work if I put 'docs/conf.py' to collect_ignore instead?

P.S. This test collection error is not triggered by --doctest-modules because it only matches *.txt files.

Do you really want conf.py to be compatible with Python 2.7?

Python 2 support has been dropped in master, so Python 3.5 is the minimum. I'd be okay with skipping collection of docs/* on Python 3.5, but I don't want to skip collection of it on the latest stable Python (or whatever Pythons are required to build docs).

Probably we're better off just to push on dropping support for Python 3.5.

Did you know you can now enable RTD integration with PRs too?

I did not. I'd be open to replacing or supplementing the Netlify integration with RTD previews.

Everything that executes Sphinx config seems to be running Python 3.8. So do you still want this reverted?

Allowing --doctest-modules on docs/conf.py also means that file gets linted with flake8 and other plugins applied. So there is value in including it in the test suite, and I'd want a compelling reason to deviate from that approach for this one project.

Would it work if I put 'docs/conf.py' to collect_ignore instead?

Yes, conditional on Python 3.5, or just use Python 3.5-compatible syntax until support is dropped. Thanks.

This test collection error is not triggered by --doctest-modules because it only matches *.txt files.

Not according to my experience or the examples in the docs. "modules" means Python modules in addition to any files matching doctest-glob.

I'd be open to replacing or supplementing the Netlify integration with RTD previews.

You can see an example of the Build pull requests for this project toggle enabled @ https://readthedocs.org/dashboard/cheroot/advanced/ + docs are here https://docs.readthedocs.io/en/latest/guides/autobuild-docs-for-pull-requests.html. It's been public for a few months but I've been using it earlier in limited beta and it works fine unless you specify extra env vars in their UI (then they are unavailable).

Not according to my experience or the examples in the docs. "modules" means Python modules in addition to any files matching doctest-glob.

I said *.txt because the docs say that the default is test*.txt and pytest.ini extends that with --doctest-glob=pkg_resources/api_tests.txt that adds one more file. Ans so it's safe to say that only .txt files will be evaluated by doctest. But what I originally meant is that the built-in test collection mechanism hits conf.py w/o any interactions with doctest.

also means that file gets linted with flake8 and other plugins applied

Ah, I had no idea that flake8 is integrated via pytest... I'll make all the necessary changes.

webknjaz · 2020-09-20T19:30:53Z

Rendering seems to be fine: https://deploy-preview-2395--ecstatic-morse-dada75.netlify.app/developer-guide.html#making-a-pull-request

webknjaz · 2020-09-21T09:01:42Z

@jaraco what's your take on this?

jaraco · 2020-09-24T14:06:01Z

+the library *since the previous version*. You should also use
+reStructuredText syntax for highlighting code (inline or block),
+linking parts of the docs or external sites.
+At the end, sign your change note by adding ``-- by


I'm not a fan of adding more requirements to the process. Users are already required to double- or triple-describe their change (in the commit, in the changelog, and in the PR). I'd like to limit the additional variance here. The link to the issue/PR and the author in the commit is more than enough attribution. I want to make changes as lightweight and automated as possible and this directive works against that goal.

How about not requiring the directive but having an option to add it if the person wants to?

I'm fine with that.

This change adds a role that links to the GitHub user Sponsors page. If that page is not set up, it'll redirect to the GitHub user profile page instead: Links to https://github.com/sponsors/{{ username }} open as GitHub Sponsors page if the target `username` has it enabled and redirect to https://github.com/{{ username }} if it's disabled.

But only under Python 3.5!

This type is more appropriate for snippets containing shell commands with leading prompts followed by their output. `bash` syntax used earlier treats everything as a raw shell script contents highlighting words like `for`.

This change places a `README.rst` document under `changelog.d/` dir in order for GitHub to render it when users navigate to this location via the web UI. It also includes that into the dev guide in Sphinx docs.

rami0r · 2020-09-25T00:44:43Z

:py:func:setuptools.setup() <setuptools.setup>_

webknjaz · 2020-10-12T18:28:15Z

@jaraco mind revisiting this? I'd rather avoid having to solve conflicts if possible :)

jaraco · 2020-10-12T21:07:53Z

All good. Thanks for your patience.

jaraco · 2020-10-15T00:08:28Z

The addition of README.txt is preventing releases.

setuptools master $ tox -e finalize
finalize installed: bump2version==1.0.1,click==7.1.2,incremental==17.5.0,Jinja2==2.11.2,MarkupSafe==1.1.1,pip==20.2.3,setuptools==50.3.0,toml==0.10.1,towncrier==19.2.0,wheel==0.35.1
finalize run-test-pre: PYTHONHASHSEED='900067985'
finalize run-test: commands[0] | python tools/finalize.py
Reading config file .bumpversion.cfg:
[bumpversion]
current_version = 50.3.0
commit = True
tag = True

[bumpversion:file:setup.cfg]

Attempting to increment part 'patch'
Values are now: major=50, minor=3, patch=1
Dry run active, won't touch any files.
New version will be '50.3.1'
Asserting files setup.cfg contain the version string...
Would write to config file .bumpversion.cfg:
[bumpversion]
current_version = 50.3.1
commit = True
tag = True

[bumpversion:file:setup.cfg]


Would prepare Git commit
Would add changes in file 'setup.cfg' to Git
Would add changes in file '.bumpversion.cfg' to Git
Would commit to Git with message 'Bump version: 50.3.0 → 50.3.1'
Would tag 'v50.3.1' with message 'Bump version: 50.3.0 → 50.3.1' in Git and not signing
Cutting release at 50.3.1
Traceback (most recent call last):
  File "/Users/jaraco/code/public/pypa/setuptools/tools/finalize.py", line 79, in <module>
    check_changes()
  File "/Users/jaraco/code/public/pypa/setuptools/tools/finalize.py", line 69, in check_changes
    assert all(
AssertionError
ERROR: InvocationError for command /Users/jaraco/code/public/pypa/setuptools/.tox/finalize/bin/python tools/finalize.py (exited with code 1)
_______________________________________________________ summary ________________________________________________________
ERROR:   finalize: commands failed

webknjaz · 2020-10-15T12:02:56Z

Ah... So there was something I overlooked.

webknjaz commented Sep 20, 2020

View reviewed changes

eme-p approved these changes Sep 21, 2020

View reviewed changes

jaraco reviewed Sep 24, 2020

View reviewed changes

Comment thread docs/conf.py

jaraco reviewed Sep 24, 2020

View reviewed changes

Comment thread changelog.d/README.rst Outdated

webknjaz added 4 commits September 24, 2020 22:58

🚑 Exclude sphinx conf from pytest auto-discovery

bb30728

But only under Python 3.5!

Fix command block in dev guide to shell-session

2754213

This type is more appropriate for snippets containing shell commands with leading prompts followed by their output. `bash` syntax used earlier treats everything as a raw shell script contents highlighting words like `for`.

📝 Add an illustrative explanation of change notes

0d69205

This change places a `README.rst` document under `changelog.d/` dir in order for GitHub to render it when users navigate to this location via the web UI. It also includes that into the dev guide in Sphinx docs.

webknjaz force-pushed the docs/changelog-fragments branch from a2d2aa4 to 0d69205 Compare September 24, 2020 20:58

webknjaz commented Sep 24, 2020

View reviewed changes

Comment thread changelog.d/README.rst

webknjaz commented Sep 24, 2020

View reviewed changes

Comment thread changelog.d/README.rst

jaraco merged commit df9157c into pypa:master Oct 12, 2020

jaraco added a commit that referenced this pull request Oct 15, 2020

Exempt README as well. Ref #2395.

f0878e9

Uh oh!

Conversation

webknjaz commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Pull Request Checklist

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!

webknjaz commented Sep 20, 2020

Uh oh!

webknjaz commented Sep 21, 2020

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!

Uh oh!

Uh oh!

Uh oh!

rami0r commented Sep 25, 2020

Uh oh!

webknjaz commented Oct 12, 2020

Uh oh!

jaraco commented Oct 12, 2020

Uh oh!

jaraco commented Oct 15, 2020

Uh oh!

webknjaz commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

webknjaz commented Sep 20, 2020 •

edited

Loading