Skip to content
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

[MAINT] Update PR template #1239

Merged
merged 6 commits into from
Aug 14, 2018
Merged

[MAINT] Update PR template #1239

merged 6 commits into from
Aug 14, 2018

Conversation

effigies
Copy link
Member

@effigies effigies commented Aug 9, 2018

Changes proposed in this pull request

As the guidelines are intended for the PR submitter, and not the reviewer, I added them to the comments, and changed the section name to "Pull request agreement".

Additionally, I made the final checklist item something everyone can check. This will avoid the "3 of 4" items complete indicator in the pull request list.

Documentation that should be reviewed

Pull-request guidelines:

  • I have read the guidelines for contributions.
  • I understand that my contributions will not be merged unless the work is
    finished (i.e. no [WIP] tag remains in the title of my PR) and tests pass.
  • The proposed code follows the
    coding style,
    to the extent I understood them (and I will address any comments raised by the PR's reviewers in this regard).
  • (Optional) I opt-out from being listed in the .zenodo.json file.

As the guidelines are intended for the PR submitter, and not the reviewer, I added them to the comments, and changed the section name to "Pull request agreement".
Additionally, I made the final checklist item something everyone can check. This will avoid the "3 of 4" items complete indicator in the pull request list.
@effigies effigies requested a review from oesteban August 10, 2018 17:48
@oesteban
Copy link
Member

I think @chrisfilo was not very convinced about the former template, so maybe this is a good time to reconsider whether we want a thorough PR template or something lighter.

That said, these changes look good to me.

**Please review and check the following**:
<!-- replace the empty checkboxes [ ] below with checked ones [x] accordingly -->
Please review and check the following:
(Replace the empty checkboxes [ ] below with checked ones [x] accordingly) -->

- [ ] I have read the [guidelines for contributions](https://github.com/poldracklab/fmriprep/blob/master/CONTRIBUTING.md).
- [ ] I understand that my contributions will not be merged unless the work is
finished (i.e. no ``[WIP]`` tag remains in the title of my PR) and tests pass.
- [ ] The proposed code follows the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these two line breaks (64 and 65). Apparently, markdown does not like them (and fixing indentations won't help)

@chrisgorgo
Copy link
Contributor

The concept of a pull request agreement seems quite demanding and off putting to me (especially for new contributors). This makes senses for large projects with a lot of 'random' contributions. We are not in such situation. However, I showed this to some other people and not everyone sees it the way I do (as in I could be exaggerating).

Imagine I modified FMRIPREP for my own purposes and decided to send a pull request, because its just a button click away. I see The Agreement. Things are serious. This is a serious project that only takes perfect contributions. I was always worried my code is not great (impostor syndrome) so probably not good enough for this serious project. I decide not to open the pull request.

Personally I would skip the agreement to maximize the number of pull requests we get - even if the quality is not perfect. As for zenodo we can use a bot such as https://github.com/hoodiehq/first-timers-bot that would detect a first time contributor and explain the concept of zenodo asking to add their name.

@effigies
Copy link
Member Author

That seems reasonable to me. I'll see if I can make it a little friendlier.

@emdupre
Copy link
Collaborator

emdupre commented Aug 12, 2018

Just to jump in here -- I totally appreciate and agree with @chrisfilo's concern that the existing PR template is not newcomer-friendly.

That said, I personally find PR templates really valuable in that they provide a kind of final check or reminder for a project's contribution process. Ideally, the development process should be highlighted across the project's documentation (sphinx site, readme, contributing guide, etc), and if the PR template is closely aligned with those then I think it can actually encourage new contributors by confirming that they've understood the project structure.

As a new developer on a project, I've found that it's very important to know what's expected from me (e.g. should I link to existing issues ?) and what I can expect in return (e.g., what's the timeline for a response ?), so having a PR template as part of that (friendly, welcoming) ethos can be a nice addition.

Really tuning the language to be clear and accessible is obviously a major part of this, and I'm happy to help iterate on this if that'd be useful !

@oesteban
Copy link
Member

I like a lot the idea of using a bot to let first-time contributors know they can add their names to the zenodo file, with the explanation we currently have in the PR template. Although I think the first-timers-bot is not exactly what we want (I found all that first timers issue a bit convoluted), we just need to send a comment with the invitation to first-time PRs.

Other than that, I'm happy with making it friendlier.

@oesteban
Copy link
Member

Okay, I just found this: https://github.com/apps/welcome

With that app we can make the interaction with first-time contributors a lot friendlier. For now, I'll set up the zenodo part for PRs.

@effigies
Copy link
Member Author

Okay, I revamped it substantially. Please feel free to criticize.

We should add some bits about licensing and tagging PRs to CONTRIBUTING.md, otherwise I'm lying about summarizing.

@oesteban
Copy link
Member

The bot is up! #1245 (comment)

Let's add the configuration of the bot (https://github.com/poldracklab/fmriprep/blob/master/.github/config.yml) to this PR, as I can see a large overlap between the final comment in your latest commit with the purpose of this bot.

With this new bot we can also send welcome messages to first issues and to first merged PRs. Just in case you can think of something we should add.

CONTRIBUTING.md Outdated
@@ -54,6 +54,11 @@ It can also be helpful to test your changes locally, using an [FMRIPREP developm

A member of the development team will review your changes to confirm that they can be merged into the main codebase.

Pull requests titles should begin with a descriptive prefix: ``ENH`` (enhancement), ``FIX`` (bug fix), ``TST`` (testing),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this a bullet-point list to be clear that these are the available options ? Maybe something like this: http://tedana.readthedocs.io/en/latest/contributing.html#pull-requests

@oesteban oesteban changed the title MAINT: Update PR template [MAINT] Update PR template Aug 14, 2018
@oesteban
Copy link
Member

I think @chrisfilo might not be fully available these days. Since I think the template is now better aligned with his comments, I'll go ahead and merge. Happy to revise again if needed 👍

@oesteban oesteban merged commit f5df948 into master Aug 14, 2018
@oesteban oesteban deleted the doc/pr_template_update branch August 14, 2018 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants