[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 
>>requirements
>>for news fragments.  Since we introduced this part of the workflow, 
>>the
>>review requirements for these has been a little unclear.  Many 
>>reviewers
>>haven't required that the news fragment be reviewed (that is, if it 
>>was
>>missing, they would say "Please add a news fragment and then merge.") 
>>or
>>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 
file...

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
   "ClientFactory.clientConnectionFailed" and
   "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 
expressions.

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
   twisted.application.app.ApplicationRunner.getLogObserver method.
   (#4092)

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 
>>do
>>a release.  This is a very simple part the development process but
>>effort here has a disproportionately large effect on the perception of
>>Twisted.
>
>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 
>fragment.

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 
>fragment text?

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 
more examples.
>
>For complicated bugfixes it's also sometimes awkward, because it's hard 
>to briefly describe a subtle, but still potentially important change in 
>behavior.
>
>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 :).

Jean-Paul



More information about the Twisted-Python mailing list