[Twisted-Python] Review process, news fragments
exarkun at twistedmatrix.com
exarkun at twistedmatrix.com
Sun Jun 20 18:26:37 EDT 2010
On 10:32 am, glyph at twistedmatrix.com wrote:
>On Jun 19, 2010, at 6:08 AM, exarkun at twistedmatrix.com wrote:
>>In particular, I'd like to draw everyone's attention to the
>>for news fragments. Since we introduced this part of the workflow,
>>review requirements for these has been a little unclear. Many
>>haven't required that the news fragment be reviewed (that is, if it
>>missing, they would say "Please add a news fragment and then merge.")
>>haven't kept the guidelines these files in mind when reviewing them.
>Can you point to some specific examples in the current release, and
>note what they should have been?
Sure. Just going in order that they happened to appear in the news
This one is too long.
- Added new "endpoint" interfaces in twisted.internet.interfaces,
which abstractly describe stream transport endpoints which can be
listened on or connected to. Implementations for TCP and SSL
clients and servers are present in twisted.internet.endpoints.
Notably, client endpoints' connect() methods return cancellable
Deferreds, so code written to use them can bypass the awkward
"Connector.stopConnecting" methods, and handle errbacks from or
cancel the returned deferred, respectively. (#1442)
I would have said something like:
- A high-level client and server setup API, "endpoints", has been
introduced which provides many benefits over using reactor.connect*
and reactor.listen* methods directly.
Alternatively, just the first two sentences of the original news
fragment stand fairly well on their own, and I probably wouldn't have
complained if the last sentence had been omitted.
Here's one that's complete nonsense.
- Substrings are escaped before being passed to a regular expression
for searching to ensure that they don't get interpreted as part of
the expression. (#1893)
The author clearly wasn't expecting this to be read outside the context
of the specific ticket being resolved, but news fragments must be
coherent on their own. This turns out to be a fix for Twisted Web's
rendering of Failures that include frames including generator
The parenthetical in this one seems to add little.
- Removed twisted.application.app.ApplicationRunner.startLogging,
which has been deprecated (doesn't say since when), as well as
support for the legacy
Omitting it entirely would have conveyed the same amount of information.
For the deprecations/removals section in particular, we definitely need
some more guidelines about what information to include and how to
construct the message. There are a few styles represented in the
entries for this release. Some include information about when the
removed API was first deprecated, but some do it by version number and
some do it by date. Others omit the information entirely. We also
switch at random between "was" and "has been". And some deprecation
messages point at a replacement API, and some don't.
Obviously this is English prose we're talking about here, and there's a
lot of room for variation. I don't want to impose a *strict* style
guide here. I do want the fragments to all aim for roughly the same
level of detail, and they should all at least make sense to a reader who
hasn't looked at the history of the relevant ticket. And where it makes
sense, I'd like the presentation of the information (like the age of the
deprecation on an API being removed or changed) to be uniform.
>>I'd like for this to change. Informative, consistently written news
>>fragments result better information being available to users when we
>>a release. This is a very simple part the development process but
>>effort here has a disproportionately large effect on the perception of
>Generally speaking, I agree. But I think we need to nail down a very
>specific proposal if this is to improve. What should reviewers be
>looking for in a good news fragment? What are pitfalls to avoid?
>Asking reviewers to look at something that isn't clearly spelled out
>may just result in more churn in reviews; I'd hate to see an extra
>round trip on a ticket because of an oxford comma placement in the news
True. Although man, what kind of a jerk would leave out the oxford
comma (or worse, ask for one to be removed!) in a news fragment?
Sheesh. I think we're all a little bit beyond that.
>>If the guidelines for news fragment content on the review process wiki
>>page are unclear or insufficient, please complain and we can work on
>>improving the weaknesses.
>There's very little in the way of stylistic guidance on this page.
>For one thing, I suspect there were a lot of fixes which could have
>been a '.misc' but were instead a '.feature' or '.bugfix'. We need
>some better guidelines for what exactly constitutes a change that is
>interesting to users.
I have little hope of this. I think we could update the page to say *at
all* that only changes interesting to end-users reading the release
announcement should be summarized. ReviewProcess#Newsfiles implies
this, but doesn't explicitly state it.
>It advises that a single sentence be written about a change to an FQPN.
>This is hard to do well for new features. For example,
>"twisted.internet.endpoints now ... exists" doesn't really scan, and
>doesn't really emphasize the importance of the feature relative to some
>of the other small fixes which have been added. I feel like I probably
>wrote too much of a dissertation in 1442.feature and not enough of a
>description in 990.feature, but I don't know what would have been
>better. If you know something should be a highlight, how should that
>be indicated? Should the highlight text be different from the news
Why did you want to include non-"highlight" text in the news fragment?
Just leave it out entirely. The news fragment is the highlight. Or am
I missing the point?
For the earlier point, I agree that the guide doesn't fit all types of
changes well. We should allow for a few different forms, and present
>For complicated bugfixes it's also sometimes awkward, because it's hard
>to briefly describe a subtle, but still potentially important change in
>A list of 10 examples of "good" feature descriptions and "good" bugfix
>descriptions would go a long way towards improving this.
>Thanks for raising the issue though. I would also like to see a more
>coherent NEWS file for 10.2 :).
More information about the Twisted-Python