Repository navigation
Conversation
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
i made a number of small suggestions, but this otherwise looks great to me!
thanks for picking this up again will
This prevents making invalid github issue links and errors out on invalid newsfragment types Co-authored-by: Brad Warren <[email protected]>
Co-authored-by: Brad Warren <[email protected]>
|
@bmw i think your CI commit looks good, want me to cherry-pick that in here or wanna merge it separately? |
|
go ahead and grab it! |
|
this lgtm! you're up @ohemorange |
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
In https://github.com/certbot/certbot/pull/10378/files, multiple entries are added to the changelog. What would be the correct way to do that post towncrier?
towncrier has the all_bullets option, which is a not very helpful name.
given the fragment 10339.fixed (reused existing for testing):
remove tls_alpn
- removed this class
- removed the other class
as-is, it generates:
- remove tls_alpn
- removed this class
- removed the other class
([#10339](https://github.com/certbot/certbot/issues/10339))
with all_bullets=false, it generates:
- remove tls_alpn
- removed this class
- removed the other class ([#10339](https://github.com/certbot/certbot/issues/10339))
personally I like the keep it as-is version better. if you agree, this comment can serve as a reference for future viewers of this pr and we can leave it at that.
|
@ohemorange i suppose our guidance could be to make all newsfragments one-liners, since IMO the default multiline behavior is pretty wack, and disabling |
|
I agree the default multiline behavior is a little silly, but requiring all oneliners also seems bad. how would you do #10378 as a one-liner? |
* Removed `acme.challenges.TLSALPN01Response`, `acme.challenges.TLSALPN01`,
`acme.standalone.TLSServer`, and `acme.standalone.TLSALPN01Server`.
Also removed parameter `alpn_protocols` from `acme.crypto_util.probe_sni`it's not good!! |
|
so i guess fragment: changelog result: ### Changed
- Removed several deprecated classes
- Removed `acme.challenges.TLSALPN01Response`
- Removed `acme.challenges.TLSALPN01`
- Removed parameter `alpn_protocols` from `acme.crypto_util.probe_sni`
- Removed `acme.standalone.TLSServer`
- Removed `acme.standalone.TLSALPN01Server` |
|
Multiline with long lines now looks like this: 
I agree this is solid. |
|
Not sure if you sure my comment at #10379 (comment), so bumping it here |
|
slight change to add wrapping in case @bmw wants to take another look |
|
lgtm! i'm merging this |
this PR is a follow up to my comment at #10495 (comment) i dislike that jsha chose a slightly different column limit than towncrier which caused it to generate awkwardly short lines in our changelog. i also dislike contributors having to remember/know about towncrier's wrapping behavior when writing changelog entries. i'd like us to find a way to not have to worry about this i initially looked into trying to set towncrier's line length limit to a much higher value, but after searching around https://towncrier.readthedocs.io/en/stable/configuration.html, i don't think it's configurable instead, i'm proposing here that we remove the wrapping entirely. wrapping was added in response to the discussion at #10379 (comment). every markdown viewer i'm aware of automatically nicely handles wrapping of long lines in markdown. most also automatically merge manually wrapped lines to wrap based on the user's display/settings. finally, since the changelog is automatically edited, i think it'll be rare for certbot devs to have to manually edit the file because of all this, i think whether the newsfragment was manually wrapped or not is largely irrelevant, we shouldn't worry about it, and we should let people wrap or their newsfragments or not as they see fit i suppose alternatively we could just let towncrier double/awkwardly wrap entries since i'm making the case that the wrapping doesn't matter, but i personally have a slight preference for this approach. let me know what y'all think
blast from the past! resurrects #9803 with all of @bmw's changes. i figured instead of force-pushing a basically brand new branch and obliterating the old review, i'd just start from a clean slate
fixes #8272