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
doc(changelog): reorganize towncrier #1621
Conversation
70dca33
to
4b60457
Compare
docs/changes/3.0.0.rst
Outdated
- The :attr:`~.RequestOptions.auto_parse_form_urlencoded` option is now | ||
deprecated in favor of :class:`falcon.media.URLEncodedFormHandler`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I can't comment on a file that has just been renamed, but it looks like this deprecation goes with the feature add for URLEncodedFormHandler
. I was going to ask where it should go, since there is no longer a "deprecations" section. But, now I think the correct move is to combine these fragments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as discussed on Gitter, IMHO as long as deprecations are coming from the fact that we introduce newer and improved ways of doing things, it's fine to just mention them here.
It's still a bit of an open question what to do with pure deprecations that are not any impromevents per se, but we could maybe handle the case if/when it comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Looking again at the comments on #1603, I guess I should make two fragments for that one in this new setup. |
Codecov Report
@@ Coverage Diff @@
## master #1621 +/- ##
============================================
+ Coverage 89.58% 100.00% +10.41%
============================================
Files 43 47 +4
Lines 2860 3548 +688
Branches 425 550 +125
============================================
+ Hits 2562 3548 +986
+ Misses 280 0 -280
+ Partials 18 0 -18
Continue to review full report at Codecov.
|
docs/changes/3.0.0.rst
Outdated
Misc | ||
---- | ||
|
||
- Setup towncrier to make CHANGES reporting much easier. (use-towncrier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a super big deal and a matter of taste, but would it be possible to disable these file names in the rendered output (eg (use-towncrier)
)?
Not sure if the information they convey is particularly useful for the reader. Or maybe they are useful in mentioning the issue number (?), but then we would need to make sure that all fragments start with an issue number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I misunderstood this and pushed a different change. If there is a way to suppress these file names, I don't know how. It's possible to suppress the prose content (which I did in one of the commits I just pushed, for the Misc section specifically), but that sounds like the opposite of what you want.
I do think that the intention is for this to be an issue number. Not sure if that should be the original issue or the PR that addresses it, but we might want to come up with some kind of consistent policy for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Towncrier insists on those numbers, I'm thinking we could use the original issue number, and fall back to just the PR number in those rare cases anything is noteworthy in Towncrier, but does not have an issue number.
@kgriffs @jmvrbanac @nZac any preferences?
As discussed on Gitter, we could also set up a template to something like
`#{issue} <https://github.com/falconry/falcon/issues/{issue}>`_
to automatically link to the issue numbers. That would be neat actually. And GH converts issue links to PR links if you happen to provide one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I just had a couple of nitpicks as well as open questions for discussion 👍
In any case, let's just remove the rendered output from the final PR, and this is IMHO ready to at least be moved out from Draft
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just marking as "changes requested" so we don't forget to remove the throwaway demonstration of the rendered output)
Any chance we can land this patch over the next few days? |
@csojinb what still needs to happen on this to get it over the finish line? |
Hi sorry I haven't gotten back to this yet. Looking at it now |
7bbedc7
to
37aab61
Compare
This is what the full output looks like as of 37aab61
Per my comment here, I realized after I pushed this that I'd misunderstood @vytas7's request. So, I can change it back to including the prose content. Do you want me to go through and put issue numbers on all of the fragment names? If so, should it be the PR number or the original issue number? |
Also, the build appears to be failing due to an inability to download a 3.8 archive? I don't know what to do about that |
Re the inability to download archives, that unfortunately happens from time to time on Travis. |
@csojinb did you see my reply on gitter? |
I did not, I will check in when I get home from work
…On Tue, Feb 4, 2020 at 12:32 PM Kurt Griffiths ***@***.***> wrote:
@csojinb <https://github.com/csojinb> did you see my reply on gitter?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1621?email_source=notifications&email_token=ABRVDS3UVITTBPUQIMTIUULRBGRCJA5CNFSM4J3CU57KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKYP7OY#issuecomment-582025147>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRVDSYH2LXD6PMEP4V6W4LRBGRCJANCNFSM4J3CU57A>
.
|
[copied from gitter]
@csojinb @vytas7 This sounds reasonable. Mainly I'd just like to ensure the fragment naming schema is consistent. We also need to talk about this in CONTRIBUTING and/or mention the desired format in the PR checklist template. |
@kgriffs I’m in favor of that suggestion. My personal computer has been having some troubles, so I’ve been a bit delayed in coming back to this, but I’ll try to get everything loaded up on my work machine and finish this up soon |
Perhaps this belongs in a followup PR, but there is also the issue highlighted here: I.e., it would be nice to be able to output links to the issue or PR at the end of the news fragment, but I haven't experimented enough with towncrier to know if it is possible to do this even when additional (hyphenated) descriptions are appended to the number. Outside of towncrier I suppose we could run a post-processing step to fix up things the way we want them. Anyway, we should have some plan about how/if we want to address this, since it informs the naming scheme. |
No worries; thanks for your help! |
Any chance we could get this over the finish line by this weekend? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csojinb this looks great almost as-is IMHO.
If we accept the issue number-based naming scheme, i.e., (\d+).directory.rst
, the only remaining items, in my understanding, are:
- Rename all newsfragments to follow the decided naming scheme.
- Update
CONTRIBUTING.md
explaining this scheme. - Check if
.github/PULL_REQUEST_TEMPLATE.md
needs an update.
09b0de5
to
4cf2ec0
Compare
I think I've managed to rename everything correctly. Here's the draft output again:
|
I'm testing out the docs now and making sure that the links all work properly in the GH markdown files, then I'll clean up the git history and I think this should be g2g |
- Rename categories and extensions to match previous changelogs - Reorganize existing fragments into categories - Combine fragments for same issue that previously fell into different categories - Remove duplicate platform support change notes - Document newsfragment naming rules - Add issue format template, with github issue links
News fragments now named for either the originating github issue, if there is one, or the PR issue number if there is not
- fix some cross references that were broken by subsequent commits and missed because the news fragments don't get built into the docs - add cross references to several news fragments
4cf2ec0
to
801a435
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great now! 👍 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried actually rendering the docs and they look great. 👍
I've just taken the liberty of mv
ing one issue number that look misplaced to me.
@vytas7 thanks for fixing the issue number! is there anything else that you need from me here, or can I just leave it to y'all to get merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Reorganize towncrier to match previous changelogs
Related Issues
N/A
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
docs/
.docs/
.versionadded
,versionchanged
, ordeprecated
directives.docs/_newsfragments/
. (Runtowncrier --draft
to ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.