On 10:32 am, glyph@twistedmatrix.com wrote:

On Jun 19, 2010, at 6:08 AM, exarkun@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

On Mon, Jun 21, 2010 at 3:12 PM, James Y Knight foom@fuhm.net wrote: ...

I think the only real way to ensure this is to have a human edit the information after it's concatenated together, before release. Having pre-written news fragments from all the authors of the changes makes that task a lot easier, but I think you really need to see everything together in one file and have a single person apply some editorial judgement to the result to have it come out well.

You may be right. We should be very reluctant to add steps to the release process though.

jml