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

During docs builds, leave newsfragments unchanged #3086

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

TeamSpen210
Copy link
Contributor

@TeamSpen210 TeamSpen210 commented Sep 11, 2024

Fixes #3084: This makes building docs include the changelog, while keeping all source files unchanged. This does that by restoring the working tree copy and any staged changes afterwards. Ideally there'd be an option to have towncrier not git add the changed copy, or write to another location, then we wouldn't have to restore. But this works fine.

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (fee3ec0) to head (1beebbf).
Report is 239 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3086      +/-   ##
==========================================
+ Coverage   99.59%   99.60%   +0.01%     
==========================================
  Files         121      121              
  Lines       17882    17882              
  Branches     3214     3214              
==========================================
+ Hits        17809    17811       +2     
+ Misses         51       50       -1     
+ Partials       22       21       -1     

see 1 file with indirect coverage changes

@TeamSpen210 TeamSpen210 requested a review from jakkdl September 11, 2024 08:00
@TeamSpen210 TeamSpen210 changed the title #3084: During docs builds, leave newsfragments unchanged During docs builds, leave newsfragments unchanged Sep 11, 2024
@jakkdl
Copy link
Member

jakkdl commented Sep 11, 2024

Looks good!

Though I just realized a different way of achieving this that's perhaps even better/simpler - move the newsfragments/ directory into docs/source (or include newsfragments/ as an additional source dir... somehow). I don't think we actually gain anything by injecting the content into changelog.rst.
That leaves warnings for document isn't included in any toctree, but if there's no easy way of filtering those out we could either [conditionally] include them in the toctree (which won't make any difference to final docs) or let the developer simply ignore those warnings

@TeamSpen210
Copy link
Contributor Author

That would check syntax, but this way lets you fully preview how the changelog would appear in context. Sphinx has heaps of events, there definitely would be a way to include newsfragments/ directly + add to the toctree.

@jakkdl
Copy link
Member

jakkdl commented Sep 12, 2024

Day-to-day I usually only care about syntax, previewing in context would be if you care about the specific way towncrier incorporates the fragments. But I suppose others might find that useful at times

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

we can also open an issue/PR in towncrier for a command-line argument that disables staging the changelog

@jakkdl
Copy link
Member

jakkdl commented Sep 18, 2024

I want this now for working on #3081, so I'll merge it :)

@jakkdl jakkdl merged commit b834f73 into python-trio:main Sep 18, 2024
40 checks passed
@jakkdl
Copy link
Member

jakkdl commented Sep 18, 2024

Works great! One downside is that the sphinx output will refer to non-existent lines, but shouldn't be very difficult to trace back to where they came from - though could cause confusion in the future if people aren't aware that this is happening behind the curtains.

.../docs/source/history.rst:14: WARNING: undefined label: 'saved_relative_timeouts'
.../docs/source/history.rst:28: WARNING: undefined label: 'saved_relative_timeouts'

@TeamSpen210
Copy link
Contributor Author

Hmm. Actually, looking for how to solve that gave me an idea for a totally different way to do this that would simplify the git bit, and solve a few problems. We have towncrier "modify" a blank file to get the changelog it produces, then use an RST include directive to insert that into the history file dynamically. Then the line numbers will be correct, we don't have to revert the real history, and as a bonus you'd be able to inspect the new file to see the new source.

@jakkdl
Copy link
Member

jakkdl commented Sep 18, 2024

Hmm. Actually, looking for how to solve that gave me an idea for a totally different way to do this that would simplify the git bit, and solve a few problems. We have towncrier "modify" a blank file to get the changelog it produces, then use an RST include directive to insert that into the history file dynamically. Then the line numbers will be correct, we don't have to revert the real history, and as a bonus you'd be able to inspect the new file to see the new source.

The downside to that would be that you now have a dangling dead file you 1. "need" to clean up, and 2. you can confuse yourself by editing the generated file instead of the original fragments, and 3. getting an error in there requires you to open the file, identify the line that caused the error, figure out which newsfragment file that corresponds to, and then go edit it.
These are fairly marginal, and probably better than the status quo, but I think for my personal day-to-day of wrangling with references and formatting in newsfragments it'd be better to directly check the fragments. Needing to debug how towncrier combines fragments is far less useful to me personally

@TeamSpen210
Copy link
Contributor Author

I was thinking the dangling file would be gitignored, so you could just leave it, and it'd have a comment at the start to tell you not to edit. The filename being not as useful could maybe be solved by even more indirection, have the temp history file include the original newsfragements. That'd definitely need to be implemented in towncrier, and it also is a little over complicated...

@TeamSpen210 TeamSpen210 deleted the restore-towncrier branch September 28, 2024 02:06
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.

Make it easier to check the formatting of a newsfragment locally
2 participants