[WIP] Added new PR template. #1010

percygautam · 2020-01-15T20:42:59Z

Added a new PR template.

percygautam · 2020-01-15T20:43:25Z

@OriolAbril Kindly check if something needs to be changed or added.

OriolAbril

I like a lot the matplotlib template, I would make some changes to make it a little more similar.

I think my key points can be summarized in: PR should be informative yet not scary for novices while also being complete yet not too much of an overhead for contributors. Therefore I would include the links in a comment (like it is done in matplotlib's template) and in the checklist section I would try to use more concise and clear points (e.g. part or all of the pull request checklist (see also next paragraph)).

Having said that, I had not thought about having several templates until I have seen you change type section and now I really like the idea. How do you feel about creating 3 templates? They could have a common skeleton and then their checklists could diverge.

Documentation

In docs improvements, it generally is important to know numpydoc style, whereas unit tests or pylint are not really relevant.

Bug fix

Here documentation may not be relevant, whereas a unit test checking the issue does not appear again is.

New feature/enhancement

This will generally need both tests and docs

Note 1: I also like the first line of xarray template:

<!-- Feel free to remove check-list items aren't relevant to your change -->

Note 2: I think (we can merge like this and see what happens) that the folder should be named PULL_REQUEST_TEMPLATE.

percygautam · 2020-01-16T13:31:59Z

@OriolAbril I think it's a good idea. I haven't yet seen any other organization yet having separate templates for pull request. I found this guide here, the folder should be named PULL_REQUEST_TEMPLATE.
Also, the commented guides and links will remove much hassle for contributors, and at the same time helps the new contributors.
I'll create the skeleton template first then diverge it into three sub-branches.

OriolAbril

Some comments that generally apply to all templates. This looks really good, just trying to find the balance between completeness and simplicity.

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

OriolAbril

Some minor comments. Thanks for all the work, this looks great and keeps getting better

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

OriolAbril · 2020-01-17T15:38:02Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+      PR format (described below)?
+- [ ] Has added unit tests to prevent issue recurrence( using [pytest fixture pattern](
+      (https://docs.pytest.org/en/latest/fixture.html#fixture))?
+- [ ] Is code style correct (are you using pylint, black)?


(follows pylint, black guidelines)

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

OriolAbril · 2020-01-17T19:11:46Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+
+- [ ] Does this PR follow the [official](https://github.com/arviz-devs/arviz/blob/master/CONTRIBUTING.md#pull-request-checklist) 
+      PR format?
+- [ ] Has added unit tests to prevent issue recurrence( using [pytest fixture pattern](


fix parentheses

Typo: Does this PR follows...

I found the second point a little bit hard to read (maybe because I am not a native speaker). Could we use a more direct way to say it, maybe something like.

Does the PR include new or updated tests to prevent issue recurrence?

It will be " Does this PR follow.....". After using does, the 's' after the verb is not used (confirmed by my editor).
And, I have changed the second point as @aloctavodia said.

OriolAbril · 2020-01-17T19:12:39Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+      PR format?
+- [ ] Has added unit tests to prevent issue recurrence( using [pytest fixture pattern](
+      (https://docs.pytest.org/en/latest/fixture.html#fixture))?
+- [ ] Is code style correct (follows pylint, black guidelines?


also parentheses and maybe "pylint and black", sorry my initial suggestion was the comma

OriolAbril · 2020-01-17T19:13:30Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+- [ ] Is code style correct (follows pylint, black guidelines?
+
+<!--
+Also, please consider reading the contributing guidelines and code of conduct  carefully before submitting the PR. It is available at 


They are available at

And I would use bullet points to ease copy pasting the links

Yep, makes sense.

aloctavodia · 2020-01-17T19:56:18Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+
+- [ ] Does this PR follow the [official](https://github.com/arviz-devs/arviz/blob/master/CONTRIBUTING.md#pull-request-checklist) 
+      PR format?
+- [ ] Has added unit tests to prevent issue recurrence( using [pytest fixture pattern](


Typo: Does this PR follows...

I found the second point a little bit hard to read (maybe because I am not a native speaker). Could we use a more direct way to say it, maybe something like.

Does the PR include new or updated tests to prevent issue recurrence?

aloctavodia · 2020-01-17T20:01:52Z

.github/PULL_REQUEST_TEMPLATE/new-feature.md

+- [ ] Has added/updated unit tests where appropriate( using [pytest fixture pattern](
+      (https://docs.pytest.org/en/latest/fixture.html#fixture))?
+- [ ] Is code style correct (follows pylint, black guidelines)?
+- [ ] Has added documentation for new feature/functionality?


suggested changes:

Included a sample plot to visually illustrate your changes? (only for plot-related functions)
Is the new feature properly documented with an example?

aloctavodia · 2020-01-17T20:03:40Z

.github/PULL_REQUEST_TEMPLATE/new-feature.md

+      PR format?
+- [ ] Included a sample plot to visually illustrate your changes?
+- [ ] Do all the added functions and methods have docstrings?
+- [ ] Has added/updated unit tests where appropriate( using [pytest fixture pattern](


maybe we can change it to:

Does the PR include new or updated tests to cover the new feature?

OriolAbril

LGTM

OriolAbril · 2020-01-17T22:49:15Z

.github/PULL_REQUEST_TEMPLATE/new-feature.md

+
+- [ ] Does the PR follow [official](https://github.com/arviz-devs/arviz/blob/master/CONTRIBUTING.md#pull-request-checklist) 
+      PR format?
+- [ ] Included a sample plot to visually illustrate your changes? (only for plot-related functions)


Is a sample plot included to visually... ? This past at the beginning in the sentence sounds strange

Now that you mention it, it does sound strange. I'll change it.

OriolAbril · 2020-01-17T22:49:35Z

.github/PULL_REQUEST_TEMPLATE/new-feature.md

+
+<!--
+Also, please consider reading the contributing guidelines and code of conduct carefully before submitting the PR. They are available at 
+- https://github.com/arviz-devs/arviz/blob/master/CONTRIBUTING.md and 


I would remove the and at the end

OriolAbril · 2020-01-17T22:59:35Z

We completely forgot about the changelog, I guess it will be a good thing to put a check about this in the checklist, especially now that it is new and we are not used to using it.

percygautam · 2020-01-17T23:15:02Z

I also think it will be a good idea. I am adding a sample changelog check in the checklist for discussion.

OriolAbril

Thanks for adding the changelog check.

OriolAbril · 2020-01-18T19:52:05Z

.github/PULL_REQUEST_TEMPLATE/bug-fix.md

+- [ ] Does the PR include new or updated tests to prevent issue recurrence (using [pytest fixture pattern](
+      https://docs.pytest.org/en/latest/fixture.html#fixture))?
+- [ ] Is your code style correct (follows pylint and black guidelines)?
+- [ ] Added the change in [change log](https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md) in correct format.


In order to make the most out of having 3 PR templates, I would use something like Is fix listed in the [Maintenance and fixes](https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#maintenance-and-fixes) section of the changelog?.

Note that using the #maintenance-and-fixes will always link to the first occurrence of "Maintenance and fixes" (the second is actually #maintenance-and-fixes-1 and so on). Unless there were some typo in the changelog section title the link will always point to the Unrelease section of the changelog. A similar thing can be done with the other sections and PR templates.

@OriolAbril Okay, Should we also specify a format to add changes to changelog?

OriolAbril · 2020-01-19T21:46:37Z

.github/PULL_REQUEST_TEMPLATE/documentation.md

+- [ ] Is fix listed in the [Maintenance and fixes](https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#maintenance-and-fixes) 
+      section of the changelog?


Here this should refer to the Documentation subsection: https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#documentation instead of the maintenance and fixes.

@OriolAbril Yes, I understand. Sorry, I have not seen the structure of changelog.md before.

OriolAbril · 2020-01-19T21:47:44Z

.github/PULL_REQUEST_TEMPLATE/new-feature.md

+- [ ] Is fix listed in the [Maintenance and fixes](https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#maintenance-and-fixes) 
+      section of the changelog?


Here this should refer to the New features subsection: https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#new-features instead of the maintenance and fixes.

OriolAbril · 2020-01-19T22:56:29Z

I think it is ready to merge

minor fix

aloctavodia · 2020-01-22T13:38:18Z

Thanks @percygautam!

Added new PR template

e4ca03f

percygautam requested a review from OriolAbril January 15, 2020 20:46

OriolAbril reviewed Jan 15, 2020

View reviewed changes

Add temlates for bug-fix, documentation and new features

4b8925d

percygautam requested a review from OriolAbril January 16, 2020 17:46

OriolAbril reviewed Jan 16, 2020

View reviewed changes

changed the templates

3450af3

percygautam requested a review from OriolAbril January 17, 2020 13:42

OriolAbril reviewed Jan 17, 2020

View reviewed changes

final changes in the template

2a8a020

percygautam requested a review from OriolAbril January 17, 2020 16:54

OriolAbril approved these changes Jan 17, 2020

View reviewed changes

aloctavodia reviewed Jan 17, 2020

View reviewed changes

final changes to the PR template

cafe55c

percygautam requested review from OriolAbril and aloctavodia January 17, 2020 22:04

OriolAbril approved these changes Jan 17, 2020

View reviewed changes

added the changelog check to PR template

1f19cbc

OriolAbril reviewed Jan 18, 2020

View reviewed changes

modified changelog check

66d0844

percygautam requested a review from OriolAbril January 19, 2020 17:48

OriolAbril reviewed Jan 19, 2020

View reviewed changes

Minor changes

68a3f89

percygautam requested a review from ahartikainen January 20, 2020 10:42

Update bug-fix.md

08f7c94

minor fix

aloctavodia added 3 commits January 22, 2020 10:35

Update documentation.md

c214e1c

minor fix

Update new-feature.md

281cd21

Update new-feature.md

cc4cc6f

aloctavodia merged commit 285ed3d into arviz-devs:master Jan 22, 2020

percygautam deleted the pr_template branch January 22, 2020 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Added new PR template. #1010

[WIP] Added new PR template. #1010

percygautam commented Jan 15, 2020

percygautam commented Jan 15, 2020

OriolAbril left a comment

percygautam commented Jan 16, 2020

OriolAbril left a comment •

edited

Loading

OriolAbril left a comment

OriolAbril Jan 17, 2020

OriolAbril Jan 17, 2020

aloctavodia Jan 17, 2020 •

edited

Loading

percygautam Jan 17, 2020

OriolAbril Jan 17, 2020

OriolAbril Jan 17, 2020

percygautam Jan 17, 2020

aloctavodia Jan 17, 2020 •

edited

Loading

aloctavodia Jan 17, 2020

aloctavodia Jan 17, 2020

OriolAbril left a comment

OriolAbril Jan 17, 2020

percygautam Jan 17, 2020

OriolAbril Jan 17, 2020

percygautam Jan 17, 2020

OriolAbril commented Jan 17, 2020

percygautam commented Jan 17, 2020

OriolAbril left a comment

OriolAbril Jan 18, 2020

percygautam Jan 18, 2020

OriolAbril Jan 19, 2020

percygautam Jan 19, 2020

OriolAbril Jan 19, 2020

OriolAbril commented Jan 19, 2020

aloctavodia commented Jan 22, 2020

		- [ ] Is fix listed in the [Maintenance and fixes](https://github.com/arviz-devs/arviz/blob/master/CHANGELOG.md#maintenance-and-fixes)
		section of the changelog?

[WIP] Added new PR template. #1010

[WIP] Added new PR template. #1010

Conversation

percygautam commented Jan 15, 2020

percygautam commented Jan 15, 2020

OriolAbril left a comment

Choose a reason for hiding this comment

Documentation

Bug fix

New feature/enhancement

percygautam commented Jan 16, 2020

OriolAbril left a comment • edited Loading

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloctavodia Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aloctavodia Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril commented Jan 17, 2020

percygautam commented Jan 17, 2020

OriolAbril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OriolAbril commented Jan 19, 2020

aloctavodia commented Jan 22, 2020

OriolAbril left a comment •

edited

Loading

aloctavodia Jan 17, 2020 •

edited

Loading

aloctavodia Jan 17, 2020 •

edited

Loading