New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prompts for metrics, tests, documentation, and hypothesis to validate #1695

Merged
merged 6 commits into from Sep 24, 2018

Conversation

5 participants
@kuychaco
Member

kuychaco commented Sep 18, 2018

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Add prompts to ensure that relevant metrics, tests, and documentation are added to pull requests, as well as hypotheses to test once changes are released.

Alternate Designs

@sguthals, @smashwilson, @annthurium, and I discussed capturing these in a project board or in a tracking issue and decided to give this method a try in order to minimize the number of places we need to look for information.

Benefits

These prompts will encourage contributors and reviewers to think more holistically about the code changes being introduced and help us keep track of what we need to validate after releasing.

The metrics prompt will help us make data-driven decisions, better understand our users and their behaviors, and draw conclusions about the impact of the changes we've introduced.

The test prompt will hopefully minimize bugs and help code reviewers and future contributors understand the intention of the code introduced.

The documentation prompt will surface useful information to users, maintainers, developers, etc

Documenting all of these in PR descriptions will give maintainers a quick way to see what was introduced in a given release cycle and what needs to be validated by simply looking at the info for each merged Pull Request.

Possible Drawbacks

None that I can think of.

Applicable Issues

See alternative approaches in this project board card and tracking issue

kuychaco added some commits Sep 18, 2018

@coveralls

This comment has been minimized.

coveralls commented Sep 18, 2018

Coverage Status

Coverage increased (+1.7%) to 81.998% when pulling 104ef5b on kuychaco-patch-1 into aec3bce on master.

@annthurium annthurium self-requested a review Sep 18, 2018

@annthurium

Should we consider aligning our pull request template with atom/atom? I know @jasonrudolph made some changes to the atom/atom pr template recently with the goal of encouraging community contributors.

<!-- Describe the documentation added or improved -->
### Pre-Release Validation

This comment has been minimized.

@annthurium

annthurium Sep 18, 2018

Contributor
  • If I was a community contributor, I might be confused by this question.

  • I don't think we have hypotheses to answer for every pull request. And if we collect those hypotheses on pull requests, that doesn't make it easy to aggregate them. Instead, I think it makes the most sense to list our hypotheses in a release checklist, since that's the cadence we're aiming for with our research efforts.

<!-- What metrics are associated with this code change and what questions can they help us answer? -->
### Tests

This comment has been minimized.

@annthurium

annthurium Sep 18, 2018

Contributor

yay, so glad this is a section! Test plans are very important.

@@ -28,3 +30,25 @@ We must be able to understand the design of your change from this description. I
### Applicable Issues
<!-- Enter any applicable Issues here -->
### Metrics

This comment has been minimized.

@annthurium

annthurium Sep 18, 2018

Contributor

💯

kuychaco added some commits Sep 19, 2018

@kuychaco

This comment has been minimized.

Member

kuychaco commented Sep 19, 2018

Should we consider aligning our pull request template with atom/atom?

Good question! I checked out the new PR template on atom/atom and it looks pretty cool. I'm a bit hesitant to bring over all of the changes they made over there (especially because of the extra step of having to copy/paste templates over), but definitely open to discussing it. For now I've drawn inspiration from them and re-used some of the wording, namely in the Tests section. What do you think @annthurium? 💭

### User Experience Research
#### Hypotheses to validate prior to releasing

This comment has been minimized.

@annthurium

annthurium Sep 19, 2018

Contributor

I'm still not convinced that this is useful information to collect at the pull request level of granularity. The pull request template already feels heavy/cumbersome to fill out, for me. Probably even more so for new contributors. And are we doing anything with this information after we write it out? We're not going to have enough time to research hypotheses for every pull request. And I don't think pull requests are where our UXR findings should live because it's hard to see themes in aggregate.

At the same time, I acknowledge that you said you would find value in writing out the answers to these questions on your own pull requests. How would you feel about clearly marking these questions as optional? Optional is different from "N/A" in my opinion. "N/A" is "there are situations where this might not apply" optional is "do this if you would find it valuable." If you find value in answering these questions in writing, I don't want to stop you. But I don't think they should be required.

This comment has been minimized.

@kuychaco

kuychaco Sep 20, 2018

Member

Yeah I hear you. Definitely don't want to be increasing friction for contributors.

Definitely agree with making explicit that these are optional (you make a good point about "N/A" not conveying optionality). I'm gonna play with the wording some more and try scaling back this section quite a bit to see how it feels.

I'm also open to dropping it from the template entirely. In terms of where best to capture this information and have these discussions, @daviwil brought up a good point and suggested that RFCs are a good candidate. That said, we don't always create an RFC for our work and not every change that would benefit from UXR would justify having an RFC.

As we get into the swing of things with UXR work I'm sure other ideas for keeping track of UXR hypotheses will come up.

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from In Progress 🔧 to QA Review 🔬 Sep 21, 2018

@annthurium

sounds great! Really love the new wording. Thanks @kuychaco for being open to feedback.

@sguthals

This comment has been minimized.

Contributor

sguthals commented Sep 21, 2018

I really like this new template and the changes. Thanks team for working together on this and @kuychaco for taking the lead!

@smashwilson

Works for me 😄

@kuychaco kuychaco merged commit fb32f4d into master Sep 24, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.7%) to 81.998%
Details

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from QA Review 🔬 to Merged ☑️ Sep 24, 2018

@kuychaco kuychaco deleted the kuychaco-patch-1 branch Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment