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

Added first draft of a CONTRIBUTING file with the new checklist idea. #146

Merged
merged 8 commits into from
Mar 25, 2015

Conversation

moorepants
Copy link
Member

Fixes #145.

  • Have all reviewer comments been addressed?

@moorepants
Copy link
Member Author

@pydy/pydy-developers Here is the proposed new checklist idea.

- [ ] Have all reviewer comments been addressed?

It should be pasted in the top most comment block of the pull request and any
non-relevant questions can be removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "This checklist should be pasted ..... . It is then the duty of reviewers to check off the items as they are completed. If any questions are not relevant to your particular pull request, a reviewer will simply check it off as done".

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine. It probably saves time that way so you don't have to sort through the list at time of pasting.

@gilbertgede
Copy link
Contributor

It seems like our issues have a number of tags, e.g. "testing", "bug", "enhancement" that are general categories. Would 3 or 4 relevant checklists be a better idea? It might address some of the concerns of the people on the sympy list, especially a simpler "bug-fix" checklist.

@moorepants
Copy link
Member Author

Maybe, but that also just introduces a more complicated system. Could we try the single list system and then make multi lists if that feels necessary after the trial period?

@moorepants
Copy link
Member Author

Speaking of this, Gilbert, we need some checks for examples too, i.e.:
If this is an example:

  • Does it have an SVG figure?
  • Does it have a problem statement?
  • Does it have a run.py file?
  • etc...

@gilbertgede
Copy link
Contributor

I feel at the very least a bug checklist & all-others checklist is a good idea. Should the list also show which issue the PR is dealing with? We don't always seem to do that.

And, regarding the 2nd comment: an example checklist would be a great example of where you'd want a specific list.

I think a simple bug checklist would be great before GSoC stuff ramps up.

@moorepants
Copy link
Member Author

Should the list also show which issue the PR is dealing with? We don't always seem to do that.

It should but not all PRs are related to issues.

@moorepants
Copy link
Member Author

@gilbertgede, can you draft an example of what you are envisioning? Or add what you'd like to see on this single list?

@gilbertgede
Copy link
Contributor

For a bug fix PR:

  • Issue being fixed is listed
  • Is all of the issue being addressed, or only part? Has this been made clear?
  • Have new tests been added to show the bug no longer is an issue, or have disabled tests been turned back on?
  • Is it passing the tests? Locally? Travis CI?
  • Have all reviewer comments been addressed?

And I could see the information for the response looking like:

  • Issue 1234
  • Completely fixes issue
  • Tests test_3, test_4 in pydy/example1/test3.py enabled
  • All tests pass
  • All comments addressed

So while the checklist might look like a lot, it might be only like 20 words to address everything in that list, which I don't think is an unreasonable expectation. I feel like we might want to have the checklist at the top, and then the contributor has to make a comment with a response like this, maybe?

@moorepants
Copy link
Member Author

@gilbertgede Ok, I've updated this document to introduce several possible checklists. The reviewer can use whatever one they want to mix and match if they don't apply.

@asmeurer
Copy link

Is the idea to copy and paste these into the PR description? If so, maybe wrap them with ```. They currently show as unclickable checkboxes.

@moorepants
Copy link
Member Author

@asmeurer Thanks, fixed.

@moorepants
Copy link
Member Author

@gilbertgede Are you fine with my changes?

@jcrist
Copy link
Member

jcrist commented Mar 24, 2015

Looks good to me, +1.

@chrisdembia
Copy link
Contributor

It's not clear what is meant by checking off "are there merge conflicts?". Does that mean there are merge conflicts? If so, then checking that off will make it seem like progress has been made on completing the PR. Seems to make more sense to leave this as a bullet.

@moorepants
Copy link
Member Author

@chrisdembia Yeah, none are really worded such that it is a "grammatically correct" checklist. I tried that early on but everything got really wordy. Here is what one list would look like not as questions but as statements:

- [ ] There are no merge conflicts.
- [ ] There is a reference to the issue number in the commit message if there is a related issue. 
- [ ] Unit tests have been added.
- [ ] The PR passes tests both locally (run `nosetests`) and Travis CI.
- [ ] All public methods/classes have docstrings. (We use the [numpydoc
  format](https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt).)
- [ ] An explanation to the online documentation has been added. (`docs` directory)
- [ ] The code follows PEP8 guidelines? (use a linter, e.g. [pylint](http://www.pylint.org), to check your code)
- [ ] The new feature is documented in the [Release Notes](https://github.com/pydy/pydy/tree/contributing#release-notes)
- [ ] The code is backwards compatible? All public methods/classes must follow deprecation cycles.
- [ ] All reviewer comments have been addressed.

I can change them all to this if it is preferable.

@gilbertgede
Copy link
Contributor

I like it, and I think we're all on the same page that this is our first attempt at this, and will go through changes/improvements/etc....

I'm +1

@moorepants
Copy link
Member Author

Ok, @chrisdembia if I've addressed your comment in the latest change please checkoff the checklist and merge at will. Thanks for the review. It is interesting that the social contract changes garner so much more attention and debate than most pure code changes.

@chrisdembia
Copy link
Contributor

It is interesting that the social contract changes garner so much more attention and debate than most pure code changes.

Could be bikeshedding; it's just easier to think about maybe?

chrisdembia added a commit that referenced this pull request Mar 25, 2015
Added first draft of a CONTRIBUTING file with the new checklist idea.
@chrisdembia chrisdembia merged commit 4032ab4 into pydy:master Mar 25, 2015
@moorepants moorepants deleted the contributing branch March 26, 2015 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a PR checklist
5 participants