Contributing guidelines improvements #495

wholmgren · 2018-06-28T21:45:13Z

It's become clear to me that our contributing guidelines are insufficient, out-of-date, and/or unknown. My hope is that improved, more visible guidelines will allow for a more productive, enjoyable experience for everyone. So, here are some proposed changes. They are aimed at everyone from novice to expert, first time contributor to project maintainer. I stress that these are guidelines, not hard rules.

I wrote these guidelines in response to pull requests that are valuable but languishing, pull requests that are active but represent a huge review burden, and discussions in other issues. Please review the changes knowing that my intention is only to help set more useful, concrete guidelines. I do not mean to criticize any prior work or approaches. Nor do I expect all contributions can be made to fit within these guidelines.

That being said, the contributing page with proposed changes is rendered here:

https://wholmgren-pvlib-python-new.readthedocs.io/en/contributing/contributing.html

The diff does not quite show the first couple of unchanged sentences, but I don't want them to be forgotten:

"Encouraging more people to help develop pvlib-python is essential to our success. Therefore, we want to make it easy and rewarding for you to contribute."

Note that I also reordered the checklist in the pull request template and added a new item: "I am familiar with the contributing guidelines". My hope is that even the most frequent contributors (myself included) will occasionally choose to follow the link if it's right in front of them. The new checklist is used below, though much of it does not apply for a documentation-only update.

I also added a new .github/CONTRIBUTING.md file that only links to the rendered documentation. It's another one of those special github files, see here. I don't know if it's ultimately useful or not, but I don't think it hurts.

I am very interested in suggestions for how to improve this document.

Closes #xxxx
I am familiar with the contributing guidelines.
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Pull request is nearly complete and ready for detailed review.

mikofski · 2018-06-30T14:58:42Z

LGTM. Thanks for writing this up.

wholmgren · 2018-07-04T17:37:03Z

@thunderfish24 @adriesse any objections or suggestions for these proposed changes to contributing.rst? No worries if you don't have the time/interest, I only want to make sure that the most frequent contributors are aware of the proposal.

adriesse · 2018-07-04T17:55:14Z

Sorry, where/what is contributing.rst? (Gives you an idea what level I'm at...)

wholmgren · 2018-07-04T17:57:19Z

https://github.com/pvlib/pvlib-python/pull/495/files#diff-de7ce680a9d255c08c48841ed58d845b

The latest "official" version of the rendered documentation is here:

http://pvlib-python.readthedocs.io/en/latest/contributing.html

And the proposal in this pull request is rendered here:

https://wholmgren-pvlib-python-new.readthedocs.io/en/contributing/contributing.html

adriesse · 2018-07-04T21:28:48Z

docs/sphinx/source/contributing.rst

@@ -15,9 +19,13 @@ Here are a few ideas for you can contribute, even if you are new to
 pvlib-python, git, or Python:

 * Make `GitHub issues <https://github.com/pvlib/pvlib-python/issues>`_
-  and contribute to the conversation about how to resolve them.
+  and contribute to the conversations about how to resolve them.


In the broader sense, answering questions on stackoverflow and/or discussing on the google group would be contributions as well.

yes, absolutely!

adriesse · 2018-07-04T21:35:39Z

docs/sphinx/source/contributing.rst

+The best way to ensure that a pull request will be reviewed and merged in
+a timely manner is to:
+
+#. Start with an issue. The issue should be well-defined and actionable.


Start by creating an issue.

adriesse · 2018-07-05T07:00:05Z

Seems fine to me.

markcampanelli · 2018-07-05T14:50:34Z

I should be able to take a real look over lunch today.

markcampanelli

Thanks for improving the guidelines.

A general comment: Is there a place for using gist's instead of PR's for contributors that want to get a preliminary discussion about architecture and implementation options/choices/changes?

markcampanelli · 2018-07-05T18:23:16Z

docs/sphinx/source/contributing.rst

+There is a lot of material in this section, aimed at a variety of
+contributors from novice to expert. Don't worry if you don't (yet)
+understand parts of it.
+

 Easy ways to contribute


I think the word "how" is missing on line 18: " Here are a few ideas for you can contribute, ..."

markcampanelli · 2018-07-05T18:30:58Z

docs/sphinx/source/contributing.rst

+functions, PVSystem/Location, and ModelChain. We recommend that
+contributors focus their work on only one or two of those layers in a
+single pull request. New models are *not* required to be available to
+the higher-level API!


It's hard to see a ton of value in adding models that aren't accessible to (perhaps many?) users through the higher-level API. Perhaps touch on this point here: What is the value and/or subsequent steps to make a new model "fully" accessible?

I'm OK with what Will wrote here. At the last workshop we heard from a broad sample of users that pvlib (either MATLAB or python) serves two primary roles: as a reference library of algorithms (our original intent), and as a platform for research or custom modeling to supplement commercial applications. ModelChain is valuable, although not essential, to these use cases. So it's OK, from my point of view, to add model functions and expose them at the PVSystem/Location level but not the ModelChain level.

I think Cliff said it well. I'd add that I'm not sure that ModelChain is going to exist in anything like its current form in 3-5 years, but I am sure that most of the core functions will remain fairly similar to what they are now.

markcampanelli · 2018-07-05T18:36:29Z

docs/sphinx/source/contributing.rst

+expanding the scope of your pull request. We'd rather see small
+additions to the project's technical debt than see a pull request
+languish because its scope expanded beyond what the reviewer community
+is capable of processing.


Sometimes it's hard for reviewers to be immediately available, so the "right" amount of patience is to be expected. That said, interested reviewers should do their best to not wait until the last minute to put in their two cents.

Great point. I added your suggestion to a new section: "Pull request reviews".

markcampanelli · 2018-07-05T18:37:23Z

docs/sphinx/source/contributing.rst

+Questions about related issues frequently come up in the process of
+addressing implementing code for a pull request. Please try to avoid
+expanding the scope of your pull request. We'd rather see small
+additions to the project's technical debt than see a pull request


Better to say "well-documented additions".

markcampanelli · 2018-07-05T18:41:57Z

docs/sphinx/source/contributing.rst

+<https://www.python.org/dev/peps/pep-0008/>`_. Maximum line length for code
+is 79 characters.
+
+Documentation must be written in `numpydoc format <https://numpydoc.readthedocs.io/>`_.


I didn't know about this specific format. Thanks!

My other big issue here has been knowing that my markup is going to look and link correctly in the eventual documentation.

Added some text about building documentation on readthedocs. Not sure if it's sufficient since I can't actually walk through it because my account is already set up. I could add something about building it locally but it's somewhat cumbersome.

markcampanelli · 2018-07-05T18:43:45Z

docs/sphinx/source/contributing.rst

+Set your editor to strip extra whitespace from line endings. This helps
+keep the commit history clean.
+
+pvlib python does not use ``logging``. Remove any logging calls that you


Consider "Remove and logging calls and print statements that you..."

markcampanelli · 2018-07-05T18:45:56Z

docs/sphinx/source/contributing.rst



 Testing
 ~~~~~~~

-pvlib's unit tests can easily be run by executing ``py.test`` on the
+pvlib's unit tests can easily be run by executing ``pytest`` on the


Line 196 below says "avoid using print. Do you mean don't commit code with print statements, or is there a better way to log extra info when a test fails?

Line 206 should replace "layer" by "layer(s)", because a change could have testing ramifications in multiple layers.

clarified and fixed.

markcampanelli · 2018-07-05T18:47:54Z

docs/sphinx/source/contributing.rst

+readability of formulae.
+
+Set your editor to strip extra whitespace from line endings. This helps
+keep the commit history clean.


You might want to mention something about what happens to the PR's commit history when it is merged.

Good idea. I added a few sentences.

markcampanelli · 2018-07-05T18:55:12Z

docs/sphinx/source/contributing.rst

@@ -220,5 +319,5 @@ This documentation

 If this documentation is unclear, help us improve it! Consider looking
 at the `pandas
-documentation <http://pandas.pydata.org/pandas-docs/version/0.18.1/
+documentation <http://pandas.pydata.org/pandas-docs/stable/


What is the difference between "stable" and "latest"? It seems that sometimes "latest" refers to master, but other times to the latest tag, which might not actually be the highest tag. (For example, Docker has some subtleties here, if I recall correctly.)

most python projects configure readthedocs such that stable is the most recently released version (git tag), whereas latest uses the current state of the master branch.

markcampanelli · 2018-07-05T18:57:27Z

.github/PULL_REQUEST_TEMPLATE.md

- - [ ] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
+The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:
+
+ - [ ] Closes #xxxx


Why remove "issue"? I think that might confuse a newbie: Closes what?

The reasoning behind this change is that github will automatically close an issue if a pull request is merged that has "closes #xxxx", but github will not do that for "closes issue #xxxx". But now that I think about it I agree that the change is a net negative.

wholmgren · 2018-07-05T22:09:30Z

Thanks @adriesse and @thunderfish24 for your reviews!

A general comment: Is there a place for using gist's instead of PR's for contributors that want to get a preliminary discussion about architecture and implementation options/choices/changes?

If by "place" you mean "is it ok to share code like this?" this yes, absolutely. I've done that a number of times, though it's hard for me to say how effective it's been because I usually don't get comments on them. But if you're asking about something like an "organization gist" then, no, not that I am aware of. I only know of gist.github.com which I believe is specific to your personal account and cannot be used with organizations.

markcampanelli

Thanks for the changes.

markcampanelli · 2018-07-06T01:31:14Z

docs/sphinx/source/contributing.rst

-the corresponding issue.
+ignore. Alternatives to incomplete pull requests include creating a
+`gist <https://gist.github.com>`_ or experimental branch and linking to
+it in the corresponding issue.


I missed this first time, I think. +1

markcampanelli · 2018-07-06T01:36:08Z

docs/sphinx/source/contributing.rst

@@ -162,20 +196,39 @@ pvlib python generally follows the `PEP 8 -- Style Guide for Python Code
 <https://www.python.org/dev/peps/pep-0008/>`_. Maximum line length for code
 is 79 characters.

-Documentation must be written in `numpydoc format <https://numpydoc.readthedocs.io/>`_.
+Code must be compatible with python 2.7 and 3.4+.


Not for this PR, but do we have a plan/statement about when we will stop 2.7 support?

@thunderfish24 #501

wholmgren · 2018-07-10T02:14:53Z

I'm going to go ahead and merge, but I will be happy to consider further improvements in new issues/PRs.

wholmgren added 3 commits June 28, 2018 13:25

add stub CONTRIBUTING.md

fe37649

add new contributing guidelines

b0d797e

clean up errors, typos

e39cd8e

wholmgren added documentation development_workflow labels Jun 28, 2018

cwhanse approved these changes Jun 28, 2018

View reviewed changes

adriesse reviewed Jul 4, 2018

View reviewed changes

markcampanelli suggested changes Jul 5, 2018

View reviewed changes

review feedback

be886c7

markcampanelli approved these changes Jul 6, 2018

View reviewed changes

add documentation section

58f95e0

wholmgren merged commit ffcfc29 into pvlib:master Jul 10, 2018

wholmgren deleted the contributing branch July 30, 2018 14:41

Contributing guidelines improvements #495

Contributing guidelines improvements #495

Conversation

wholmgren commented Jun 28, 2018

mikofski commented Jun 30, 2018

wholmgren commented Jul 4, 2018

adriesse commented Jul 4, 2018

wholmgren commented Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriesse Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

adriesse commented Jul 5, 2018

markcampanelli commented Jul 5, 2018

markcampanelli 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

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

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

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

wholmgren commented Jul 5, 2018

markcampanelli 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

wholmgren commented Jul 10, 2018

wholmgren commented Jul 4, 2018 •

edited

Loading

adriesse Jul 4, 2018 •

edited

Loading