VIM-4977: JAM - Updated PR template proposal by nicolelehrer · Pull Request #125 · vimeo/VimeoNetworking

nicolelehrer · 2017-03-27T18:16:14Z

Ticket

Ticket Summary

(1) Update the PR template to facilitate more helpful PR descriptions from developers.
(2) Add a PR checklist for the contributor to fill in prior to requesting PR review

Pain points this is trying to solve

We'd like to identify some expectations we have for devs who don't contribute to our repos regularly by listing them in a PR checklist
Sometimes we copy and paste the ticket description (if applicable) inside Issue Summary. Instead let's translate the ticket / general issue into the technical problem we're trying to solve so we're not trying to figure this out while reading about implementation.
- instead of: "Build the new 'foo feature' page", do "(1) Implement a new collection view with cells that are populated by the foo object . (2) Implement cell that shrinks when not in center. (3) etc.."
- instead of "Playback was not working in fullscreen", do "Play button unresponsive in fullscreen layout because of bad constraints".
A reviewer should have a big picture idea of how you solved the problem, how your changes relate to each other, and what your code will affect. Let's cover these in Implementation summary and Reviewer tips.
A reviewer shouldn't have to guess if a large chunk of code was copied and pasted or was actually changed. Let's indicate when this case happens inside Reviewer tips.
Let's try to make testing as easy as possible on the reviewer.

The intent of these bullet points is not to make PR descriptions longer, but more specific while remaining concise. These bullet points should be deleted if not applicable.

Implementation Summary

Added bullet points and checklist to help remind developers to provide more specific info on a PR for the reviewer.

How to Test

N/A

cameo-github · 2017-03-27T18:24:56Z

	4 Warnings
⚠️	VimeoNetworking/Sources/Request+Notifications.swift#L54: result of call to ‘map’ is unused `notifications.map { (notification: VIMNotification) -> Void in`
⚠️	VimeoNetworking/Sources/VimeoClient.swift#L328: cast from ‘Response<VIMNullResponse>’ to unrelated type ‘Response<ModelType>’ always fails `let response = Response(model: nullResponseObject, json: [:]) as! Response<ModelType>`
⚠️	VimeoNetworking/Sources/Models/VIMVideoPreference.m#L41: incompatible pointer types assigning to ‘NSString * _Nullable’ from ‘VIMPrivacy * _Nullable’ [-Wincompatible-pointer-types] `privacy.view = self.privacy;`
⚠️	VimeoNetworking/Sources/Models/VIMVODItem.m#L285: incompatible pointer types returning ‘VIMConnection * _Nullable’ from a function with result type ‘VIMVODConnection *’ [-Wincompatible-pointer-types] `return [self connectionWithName:VIMConnectionNameVideos];`

	1 Message
📖	Executed 22 tests, with 0 failures (0 unexpected) in 0.301 (0.325) seconds

See build details on CircleCI

Generated by 🚫 danger

kevinzetterstrom

This is really great 😄 and I can't wait to start using this everywhere for increased clarity and ease of use. I just have a few small comments.

kevinzetterstrom · 2017-03-27T18:28:11Z

@@ -0,0 +1,36 @@
+#### Ticket (For Vimeo staff only)


As the first line of the template, do you think non-Vimeo Staff will see this and assume the entire template doesn't apply to them rather than just this section? I have gotten that feeling before when it's been included as the second line as it is within the Android public repos, like this. I'm not sure what the best approach would be to solve this if it is a problem. Perhaps reordering the section down further? - But I like having the ticket number at the top. Perhaps better copy?

kevinzetterstrom · 2017-03-27T18:30:36Z

+
+- *Outline the goals of the PR in technical terms. If this branch fixes a bug, state what caused the bug.*
+
+#### Pull Request Checklist


I think it makes sense for Issue Summary to immediately precede Implementation Summary. Pull Request Checklist may make sense to move up or down. I almost feel like moving it up would be better, so reviewers immediately know the state of the PR before investing time reading summaries. What do you think?

kevinzetterstrom · 2017-03-27T18:30:47Z

+
+#### Implementation Summary
+
+- *Identify the core components that were added or changed, and briefly state the reasoning behind these changes.*


kevinzetterstrom · 2017-03-27T18:32:53Z

+
+- *Provide the order in which files or components should be reviewed.* 
+
+- *List general components of the application that this PR will affect.*


Not sure the term application applies in this sense, since this is a library itself. Perhaps List general components that this PR will affect. would be better suited.

kevinzetterstrom · 2017-03-27T18:33:23Z

+
+- *Identify necessary conditions for testing (e.g. logged out, bad connectivity).*
+
+- *If a special type of account is needed for log in, contact a developer privately to provide credentials.


kevinzetterstrom

Looks great!

nicolelehrer · 2017-03-27T20:22:43Z

Thank you for the helpful feedback @kevinzetterstrom!

@ghking @jenniferlim @jasonhawkins @mikew-personal @supperspidey - please take a look at this PR as a proposed template for our public repos.

Also please take a glance at the original PR for private repos to confirm new changes are ok. https://github.vimeows.com/MobileApps/Vimeo-iOS/pull/1259

Once we are able to sign off, I'll update all public and private repos.

nicolelehrer · 2017-03-30T19:09:50Z

cc: @fyell

fyell · 2017-03-30T19:19:57Z

+#### Pull Request Checklist
+
+- [ ] Resolved any merge conflicts
+- [ ] No build errors or warnings are introduced


Can these 2 be assumed from CI passing?

@fyell the thought was folks shouldn't expect us to review if a build is broken or has warnings

jasonhawkins

I, along with these two folks –> 🔭👩🏻‍🔬👨🏻‍🔬🔬 – both highly respected in their fields – approve this pull request.

jenniferlim · 2017-03-30T20:14:53Z

Looking great! @nicolelehrer. 😊

fyell · 2017-03-30T20:15:37Z

Looks fantastic to me 👍

Nicole Lehrer added 3 commits March 27, 2017 13:56

port over enterprise template

73aef74

added pull request checklist

2bd299e

adapt for non-vimean contributors

e46b923

nicolelehrer self-assigned this Mar 27, 2017

Nicole Lehrer added 2 commits March 27, 2017 14:18

remove submodule section

903585d

don't encourage public credential sharing :(

66200d7

nicolelehrer assigned kevinzetterstrom and unassigned nicolelehrer Mar 27, 2017

kevinzetterstrom reviewed Mar 27, 2017

View reviewed changes

responded to PR feedback

a306e12

kevinzetterstrom approved these changes Mar 27, 2017

View reviewed changes

add missing closing asterisk, change placement of note for ticket number

d071eff

fyell reviewed Mar 30, 2017

View reviewed changes

jasonhawkins approved these changes Mar 30, 2017

View reviewed changes

nicolelehrer merged commit 0d4285e into develop Mar 30, 2017

nicolelehrer deleted the VIM-4977-jam-pr-template branch March 30, 2017 22:36

This was referenced Mar 31, 2017

VIM-4977: JAM - Updated PR template proposal vimeo/VimeoUpload#136

Merged

VIM-4977: JAM - Updated PR template proposal vimeo/PlayerKit#4

Merged


		- Outline the goals of the PR in technical terms. If this branch fixes a bug, state what caused the bug.

		#### Pull Request Checklist


		#### Implementation Summary

		- Identify the core components that were added or changed, and briefly state the reasoning behind these changes.


		- Provide the order in which files or components should be reviewed.

		- List general components of the application that this PR will affect.


		- Identify necessary conditions for testing (e.g. logged out, bad connectivity).

		- *If a special type of account is needed for log in, contact a developer privately to provide credentials.

Conversation

nicolelehrer commented Mar 27, 2017

Ticket

Ticket Summary

Implementation Summary

How to Test

Uh oh!

cameo-github commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinzetterstrom left a comment

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

kevinzetterstrom left a comment

Choose a reason for hiding this comment

Uh oh!

nicolelehrer commented Mar 27, 2017

Uh oh!

nicolelehrer commented Mar 30, 2017

Uh oh!

fyell Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolelehrer Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

jasonhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

jenniferlim commented Mar 30, 2017

Uh oh!

fyell commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cameo-github commented Mar 27, 2017 •

edited

Loading

fyell Mar 30, 2017 •

edited

Loading