-
Notifications
You must be signed in to change notification settings - Fork 15
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 pull request template #11
base: main
Are you sure you want to change the base?
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
I like the template. One thing I find useful in summaries/PR messages is if people explain why they did what they did. A bit like the philosophy of comments not saying what the code does but why the code is the way it is. However "why" is a hard question to answer and sometimes the answer is "because i don't know how else to do it/just a first guess" which people don't want to write (including me). Could we combine the first two sections (in the hopes that the list of what changed is only ever one thing anyway) and/or include a "why" question in it? |
Try and encourage people to explain why each change was made.
I've updated the template with your suggestion. I also thought about putting in an example like
but I think it's too difficult to come up with some good text so I decided not to |
Co-authored-by: Georgiana Elena <[email protected]> Co-authored-by: Tania Allard <[email protected]>
@trallard @GeorgianaElena thanks for your suggestions, what do you think now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a question about the first part of the template, but otherwise, this LGTM 🚀
Co-authored-by: Georgiana Elena <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great improvement - thanks! It looks good to me, my one concern is that it is a lot of text and I worry that some folks may not read the commented-out HTML sections, but I think it's worth trying this out and seeing what people's experience is.
I agree with you on not having too much text, and it would be nice to reduce it a bit. The problem is deciding what to strip out whilst keeping it friendly. I've added this to the agenda for the October team-meeting, so there's still time for people to make suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @manics! ✨ I left a couple of comments but approving regardless
They are all optional, but the more information you provide the easier it will be for us to review your changes. | ||
Everyone here is a volunteer, so please help us out if you can. | ||
|
||
If you want to discuss anything Jupyter related, or to meet other users and developers, please say hello on https://discourse.jupyter.org/ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add in this link as well? https://discourse.jupyter.org/t/introduce-yourself/17/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do, though since we've got the welcome bot maybe we could add the introduce-yourself link there, perhaps with a stronger emphasis on "say hello", and just have the general link here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this but would actually reverse the suggestion. This is a PR template so folks using it are probably comfortable/familiar enough to make a contribution - we would just like to get to know them better (hence "say hello"). Whereas the bot's job is to orient newcomers who may be more likely to open a support request (hence general link). What do you think?
Also I really am splitting hairs here, so feel free to pass :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quite like the finalised version :)
I think the suggestions of @sgibson91 are interesting and definitely worth thinking about (I particularly like having an explicit "user model" of what assumptions we're making about users at various intersection points with the community). That said, I also think that this is already a nice improvement. What do folks think about merging this one in as-is, and discussing the other issues that @sgibson91 brings up in another issue / subsequent PR after we've had time to see how the community responds to this PR template? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you everyone for your work on this! I think this is a great starting point! 🎉 ❤️ 🌻
I think a topic for future iteration may be about reaching some kind of agreement or similarly to making a change ahead of time before submitting a PR at all. I really dislike turning down PRs after someone has put in work learning about the code base and such, and/or asking for lots of changes, and during those situations it helps to have discussed the PR idea ahead of time.
@consideRatio I think for more complex questions that require community input and thinking, or that might have multiple potential outcomes, we can always piggy-back on the JEP process for it! We should also agree to a team mechanism for making a "team decision" when no consensus is reached (e.g. a vote process etc) |
This is a follow-up to jupyterhub/team-compass#302 and also jupyterhub/repo2docker#671, and partly related to jupyterhub/team-compass#288
The aim of this template is to obtain more information from PR submitters to help us (and external contributers willing to help review!) understand the PR. It also attempts to deal with some concerns raised in jupyterhub/team-compass#288 by encouraging people to give more context for their PR.
@trallard