search

new-xkit / XKit

Fork of XKit, the extension framework for Tumblr
https://new-xkit-extension.tumblr.com
Other
459 stars 135 forks source link

Formalize Week in Review #679

Open bvtsang opened 8 years ago

bvtsang commented 8 years ago

Currently, the Week in Review consists of all the pull requests that have been merged in the previous week, plus all the open pull requests. However, there is disagreement on whether to avoid mentioning internal/developer-specific pull requests, as they probably don't pertain to the users who ultimately read these reviews.

To fix this issue, the following need to be discussed:

Wolvan commented 8 years ago

Whatever design we decide on, I'd appreciate if you would think about the automatic creation that is happening so far. My bot currently works by reading in all closed pull requests, reading the changed extensions and versions and using the PR title as the change description for that version. Then it pulls all open PRs and lists the numbers + the titles. I suggested adding a label to PRs that we do not want to show up in the WiR. If we want to have the title that shows up changed we could have some kind of syntax in the PR body that allows us to define those things

0xazure commented 8 years ago

I am of the opinion that WiR needs to be tailored to its primary audience: users of XKit. We should not make assumptions about their technical literacy, and should therefore use plain language where ever possible.

To wit, "internal" changes (for example: #494, #573, #652) should not be surfaced in WiR. End users don't care about the internals, so this information isn't relevant.

I am still in favour of including links to the relevant PRs so that users interested in tracking the progress of an issue or resolution can easily follow up. Issues can be viewed even if the user doesn't have a GtiHub account, and seeing issue discussion could prompt them into registering and contributing to the conversation, so I'd like to continue to see these links surfaced in WiR.

As a reference, Discord's Changelogs could be a good model going forward. They talk about the changes, but also inject some humour into the notes in such as way that makes them fun to read. I also like how they split the sections into "New Features", "Fixes" and "Coming Soon". We would have to adapt this to how our functionality is split across modules, so maybe "New Features" and "Fixes" for each extension (where applicable), and then a catch-all "Coming Soon" at the end of the post for up-and-coming changes that will soon be merged.

As far as automation is concerned, I think this is one area that we want to ditch most of it. Automatically compiling a list of changes to review is helpful, but that list should be manually transformed (through editing and rewriting) into what actually goes out to users.

adding a label to PRs that we do not want to show up in the WiR

This clutters the GitHub label list for very little gain. Deleting an entry from the changelog takes so little time, and I have reservations about issues being incorrectly tagged and not making it into the list due to a mis-click on a label. In this scenario, it would be very time-consuming to track down if an issue didn't make it into the list. There are also still issues in correctly using Waffle's control labels, and I have low hopes that PRs will always be properly tagged to remove them from the WiR summary.

some kind of syntax in the PR body

This would needlessly overcomplicate submissions, and irrevocably ties us to a particular format. Also note that with how we currently use Homu, any text in the main PR body becomes the commit message from Homu's merge into master. Added more text to these messages will clutter the history, making visually grepping more difficult.

Wolvan commented 8 years ago

PR bodies become Homu's commit messages

Haven't thought about Homu's commit messages, that's a really good point

If you really want to ditch the automatic compiling I can turn that off anytime, you just gotta tell me when

bvtsang commented 8 years ago

RE internal changes: Initially I thought removing them was a form of censorship, but I agree that we should tailor the Week in Review to our audience, who may not want to know the nitty-gritty of our code.

Below is a draft presentation of the Week in Review. I'm trying to channel the playfulness from Discord's changelogs.

Week in Review: 2015 October 27 - November 2

Bug Fixes

New Features

Coming Soon

invalidCards commented 8 years ago

:+1: seems a lot more user-friendly

Wolvan commented 8 years ago

I doubt that I'm able to generate these changelog texts automatically, so that's something to keep in mind if we still want a semi-automatic way of having our changelogs put together

invalidCards commented 8 years ago

@Wolvan If you could provide just a list of "pulled" and "upcoming" we can edit the messages, the order and the headings ourselves

Wolvan commented 8 years ago

Should I do New Features and Bugfixes based on Version increase I guess?

invalidCards commented 8 years ago

Yessir

bvtsang commented 8 years ago

To be explicit:

Proposed and updated structure (which you are free to discuss):

Week in Review: Year Month Day - Month Day

Bug Fixes

New Features

Changes in Review

Wolvan commented 8 years ago

@bvtsang I'd rename Coming Soon to "Changes in review", since that is basically what it is. We can't say if something's coming soon after all

bvtsang commented 8 years ago

Good point, I'll amend that. Thanks.

0xazure commented 8 years ago

The exception is anything special

We could always have some kind of preamble, and then list the (hopefully brief) exceptions under that.

I think "developer-only" changes are invisible to the end user.

I think this will have to be handled on a case-by-case basis. Some things, like logging, are in fact visible to the (technologically savvy) end-user, but I would see that more as an internal change. I agree with the general sentiment here, but there needs to be an understanding that there should be some wiggle room. Far better to leave things out (that could possibly be added on to the next WiR should a discussion later prove that the change is user-facing) than to include them and confuse people.

we won't be able to generate the descriptions automatically

I think this is fine. Generating a list of the currently merged and future changes is more than useful enough.

caution when including something in the Coming Soon section

Agreed, this section will need more vetting than the bugfixes one. It'll be more work, but we might have to reach out to the authors (and reviewers, if they exist) of the PRs to know when the change is likely to be merged.

@bvtsang really like the work you put in to the "demo" WiR, I'm definitely feeling the whimsy. Much nicer than the spartan WiRs of yore. Let me know if there's anything I can do to assist.

Wolvan commented 8 years ago

that is why I said we should rename the Coming Soon section. It's not coming soon, it is what MIGHT come further down the line

0xazure commented 8 years ago

We may also want to consider how we distribute WiR.

See #554, #562.

Wolvan commented 8 years ago

@0xazure we can distribute the WiR over Paperboy once (IF) it gets merged. That's what the weekly news switch is for

0xazure commented 8 years ago

@Wolvan that's... why I'm bringing it up here? So that we don't forget to consider the how in addition to the what. Dissemination of the information is just as important as the information itself.

bvtsang commented 8 years ago

Something I just noticed: if we group together multiple changes to the same extension, then in our current template we can't link to the pull requests. For example, let's say say One-Click Postage has two changes associated with it: v1.0.0 and v1.0.1. Given the following formatting...

[One-Click Postage v1.0.1:](link to pull request for v1.0.1) A summary of changes up to the latest version. Wit and humor not required.

...we don't have a good place to link to the pull requests for v1.0.0 and v1.0.1.

One way around this is to write it like this:

One-Click Postage [v1.0.0](link to pull request for v1.0.0), [v1.0.1](link to pull request for v1.0.1): A summary of changes up to the latest version. Wit and humor not required.

Wolvan commented 8 years ago

I personally prefer having a short summary to each version change, not a grouped one

bvtsang commented 8 years ago

I prefer to lump them together because it's simpler to write a general summary than it is to write summaries for each version.

For example, as of this writing the most recent Week(s) in Review shows changes for versions 1.0.1, 1.1.0, 1.1.1, 1.1.2, and 1.1.3. The changes for each are as follows:

It's simpler for me to summarize the changes altogether and leave curious readers to click the links to the pull requests to find out more than it is to write individual summaries for each. To be fair, writing multiple summaries may seem simpler to someone else, but if I could choose between writing one and five descriptions, I'd write just one.

0xazure commented 8 years ago

Given that WiR is Week in Review, do we really need to explicitly split the changes up on version increments? Taking RDO as an example, why not include only the most up-to-date (latest) version number that has been pushed, and then in-line linking the relevant PRs in a single summation?

As an example:

This way we communicate the latest available version (which is the only version available to download given we don't support users reverting to old versions) to prompt people to update if they don't have that version, a good summary of the changes made since the last WiR, and links for those curious enough to poke at the GitHub issues.

bvtsang commented 8 years ago

I like your template better. It accommodates both the conveyance of whimsy and the need to link to PRs. I'll update the proposed structure above.

Next, I'll take a look at paperboy to see if we can use it for our Week in Reviews.

Wolvan commented 8 years ago

@bvtsang how would you like the autogenerated design to look like in this case?

bvtsang commented 8 years ago

Although I answered you on Gitter, I'm going to state it here for the record: the current autogenerated template is fine. It already has all the information I need to write the proposed format: a list of all pull requests grouped by extension.