-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Contributing guidelines improvements #495
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Contributing | ||
| ============ | ||
|
|
||
| We welcome your contributions! Please see the [contributing](http://pvlib-python.readthedocs.io/en/latest/contributing.html) page for information about how to contribute. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,19 @@ | ||
| pvlib python pull request guidelines | ||
| ==================================== | ||
|
|
||
| Thank you for your contribution to pvlib python! | ||
| Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below. | ||
|
|
||
| You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed: | ||
| You may submit a pull request with your code at any stage of completion. | ||
|
|
||
| - [ ] Closes issue #xxxx | ||
| - [ ] Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services. | ||
| - [ ] Code quality and style is sufficient. Passes ``git diff upstream/master -u -- "*.py" | flake8 --diff`` and/or landscape.io linting service. | ||
| - [ ] 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 | ||
| - [ ] I am familiar with the [contributing guidelines](http://pvlib-python.readthedocs.io/en/latest/contributing.html). | ||
| - [ ] 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. | ||
|
|
||
| Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above. | ||
| - [ ] 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. | ||
|
|
||
| Brief description of the problem and proposed solution (if not already fully described in the issue linked to above): | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,10 @@ 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. | ||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the word "how" is missing on line 18: " Here are a few ideas for you can contribute, ..." |
||
| ~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
@@ -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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the broader sense, answering questions on stackoverflow and/or discussing on the google group would be contributions as well.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, absolutely! |
||
| * Read issues and pull requests that other people created and | ||
| contribute to the conversation about how to resolve them. | ||
| Look for issues tagged with | ||
| `good first issue <https://github.com/pvlib/pvlib-python/labels/good%20first%20issue>`_, | ||
| `easy <https://github.com/pvlib/pvlib-python/labels/easy>`_, | ||
| or `help wanted <https://github.com/pvlib/pvlib-python/labels/help%20wanted>`_. | ||
| * Improve the documentation and the unit tests. | ||
| * Improve the IPython/Jupyter Notebook tutorials or write new ones that | ||
| demonstrate how to use pvlib-python in your area of expertise. | ||
|
|
@@ -35,6 +43,9 @@ pvlib-python, git, or Python: | |
| How to contribute new code | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| The basics | ||
| ---------- | ||
|
|
||
| Contributors to pvlib-python use GitHub's pull requests to add/modify | ||
| its source code. The GitHub pull request process can be intimidating for | ||
| new users, but you'll find that it becomes straightforward once you use | ||
|
|
@@ -53,16 +64,11 @@ process. Here's an outline of the process: | |
| request. | ||
|
|
||
| The Pandas project maintains an excellent `contributing page | ||
| <https://github.com/pydata/pandas/wiki/Contributing>`_ that goes into | ||
| detail on each of these steps. Also see GitHub's `Set Up Git | ||
| <http://pandas.pydata.org/pandas-docs/stable/contributing.html>`_ that goes | ||
| into detail on each of these steps. Also see GitHub's `Set Up Git | ||
| <https://help.github.com/articles/set-up-git/>`_ and `Using Pull | ||
| Requests <https://help.github.com/articles/using-pull-requests/>`_. | ||
|
|
||
| Note that you do not need to make all of your changes before creating a | ||
| pull request. Your pull requests will automatically be updated when you | ||
| commit new changes and push them to GitHub. This gives everybody an easy | ||
| way to comment on the code and can make the process more efficient. | ||
|
|
||
| We strongly recommend using virtual environments for development. | ||
| Virtual environments make it trivial to switch between different | ||
| versions of software. This `astropy guide | ||
|
|
@@ -73,17 +79,108 @@ environment. | |
|
|
||
| You must include documentation and unit tests for any new or improved | ||
| code. We can provide help and advice on this after you start the pull | ||
| request. | ||
| request. See the Testing section below. | ||
|
|
||
|
|
||
| .. _pull-request-scope: | ||
|
|
||
| Pull request scope | ||
| ------------------ | ||
|
|
||
| This section can be summed up as "less is more". | ||
|
|
||
| A pull request can quickly become unmanageable if too many lines are | ||
| added or changed. "Too many" is hard to define, but as a rule of thumb, | ||
| we encourage contributions that contain less than 50 lines of primary code. | ||
| 50 lines of primary code will typically need at least 250 lines | ||
| of documentation and testing. This is about the limit of what the | ||
| maintainers can review on a regular basis. | ||
|
|
||
| A pull request can also quickly become unmanageable if it proposes | ||
| changes to the API in order to implement another feature. Consider | ||
| clearly and concisely documenting all proposed API changes before | ||
| implementing any code. Modifying | ||
| `api.rst <https://github.com/pvlib/pvlib-python/blob/master/docs/sphinx/source/api.rst>`_ | ||
| and/or the latest `whatsnew file <https://github.com/pvlib/pvlib-python/tree/master/docs/sphinx/source/whatsnew>`_ | ||
| can help formalize this process. | ||
|
|
||
| Of course, sometimes it is necessary to make a large pull request. We | ||
| only ask that you take a few minutes to consider how to break it into | ||
| smaller chunks before proceeding. | ||
|
|
||
| pvlib-python contains :ref:`3 "layers" of code <modeling-paradigms>`: | ||
| 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! | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Cliff said it well. I'd add that I'm not sure that |
||
|
|
||
|
|
||
| When should I submit a pull request? | ||
| ------------------------------------ | ||
|
|
||
| The maintainers will follow same procedures, rather than making direct | ||
| commits to the main repo. Exceptions may be made for extremely minor | ||
| changes, such as fixing documentation typos. | ||
| The short answer: anytime. | ||
|
|
||
| The long answer: it depends. If in doubt, go ahead and submit. You do | ||
| not need to make all of your changes before creating a pull request. | ||
| Your pull requests will automatically be updated when you commit new | ||
| changes and push them to GitHub. | ||
|
|
||
| There are pros and cons to submitting incomplete pull-requests. On the | ||
| plus side, it gives everybody an easy way to comment on the code and can | ||
| make the process more efficient. On the minus side, it's easy for an | ||
| incomplete pull request to grow into a multi-month saga that leaves | ||
| everyone unhappy. If you submit an incomplete pull request, please be | ||
| very clear about what you would like feedback on and what we should | ||
| ignore. Alternatives to incomplete pull requests include posting a link | ||
| to an experimental branch or to a `gist <https://gist.github.com>`_ in | ||
| the corresponding issue. | ||
|
|
||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Start by creating an issue. |
||
| #. Ask the maintainers to tag the issue with the appropriate milestone. | ||
| #. Make a limited scope pull request. It can be a lot of work to check all of | ||
| the boxes in `pull request guidelines | ||
| <https://github.com/pvlib/pvlib-python/blob/master/.github/PULL_REQUEST_TEMPLATE.md>`_, | ||
| especially for pull requests with a lot of new primary code. | ||
| See :ref:`pull-request-scope`. | ||
| #. Tag ``@pvlib/maintainer`` when the pull request is ready for review. | ||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to say "well-documented additions". |
||
| languish because its scope expanded beyond what the reviewer community | ||
| is capable of processing. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point. I added your suggestion to a new section: "Pull request reviews". |
||
|
|
||
|
|
||
| Code style | ||
| ~~~~~~~~~~ | ||
|
|
||
| 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/>`_. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| pvlib python uses a mix of full and abbreviated variable names. See | ||
| :ref:`variables_style_rules`. We could be better about consistency. | ||
| Prefer full names for new contributions. This is especially important | ||
| for the API. Abbreviations can be used within a function to improve the | ||
| readability of formulae. | ||
|
|
||
| Set your editor to strip extra whitespace from line endings. This helps | ||
| keep the commit history clean. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to mention something about what happens to the PR's commit history when it is merged.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. I added a few sentences. |
||
|
|
||
| pvlib python does not use ``logging``. Remove any logging calls that you | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider "Remove and logging calls and |
||
| added during development. ``warning`` is ok. | ||
|
|
||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 196 below says "avoid using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 206 should replace "layer" by "layer(s)", because a change could have testing ramifications in multiple layers.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clarified and fixed. |
||
| pvlib directory: | ||
|
|
||
| ``pytest pvlib`` | ||
|
|
@@ -148,6 +245,7 @@ in fact, happen. Finally, we check that the output of the method call is | |
| reasonable. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| def test_PVSystem_ashraeiam(mocker): | ||
| # mocker is a pytest-mock object. | ||
| # mocker.spy adds code to a function to keep track of how it is called | ||
|
|
@@ -189,6 +287,7 @@ The example below shows how mock can be used to assert that the correct | |
| PVSystem method is called through ``ModelChain.run_model``. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| def test_modelchain_dc_model(mocker): | ||
| # set up location and system for model chain | ||
| location = location.Location(32, -111) | ||
|
|
@@ -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/ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| contributing.html>`_ for inspiration. | ||
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.
Why remove "issue"? I think that might confuse a newbie: Closes what?
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.
Agree
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.
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.