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

[DOC]: Add docstring example to the contributing docs #2254

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented Oct 11, 2024

  • Partially addresses GSoC feedback on the pvlib contribution process #2081
  • I am familiar with the contributing guidelines
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Adds a docstring example to the contributing page. Now that I came back to the conversation in #2081 I see it was a discussion between @IoannisSifnaios and @AdamRJensen , so sorry for getting into it.

I still hesitate a bit on whether this is the preferred format to include this template here, maybe just linking to well documented functions is enough.

@cwhanse
Copy link
Member

cwhanse commented Oct 11, 2024

What do we think about these directives? https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#describing-changes-between-versions

I rather like them but don't want to introduce something that's a pain to manage. If we like them, it would be nice to show one in the example.

docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

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

Nice job. I think this example will certainly help contributors in the future

docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
where :math:`a` is the result of the equation, :math:`b` and :math:`c`
are inputs.

Or a figure with a caption:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding somewhere where such figures should be stored in the pvlib directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is intuitive enough if you see the repo layout and that most of the time us contributors copy&paste from other places. Feel free to weight in 👀

docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
echedey-ls and others added 4 commits October 19, 2024 01:44
Co-Authored-By: RDaxini <143435106+RDaxini@users.noreply.github.com>
Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Copy link
Contributor

@IoannisSifnaios IoannisSifnaios left a comment

Choose a reason for hiding this comment

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

Personally this is exactly what I had in mind when we were discussing this! Nice work @echedey-ls! Some suggestions from my side.

docs/sphinx/source/contributing/style_guide.rst Outdated Show resolved Hide resolved
echedey-ls and others added 4 commits October 22, 2024 00:46
Co-Authored-By: Ioannis Sifnaios <88548539+IoannisSifnaios@users.noreply.github.com>
Co-Authored-By: Ioannis Sifnaios <88548539+IoannisSifnaios@users.noreply.github.com>
Co-Authored-By: Ioannis Sifnaios <88548539+IoannisSifnaios@users.noreply.github.com>
Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-Authored-By: Ioannis Sifnaios <88548539+IoannisSifnaios@users.noreply.github.com>
@echedey-ls echedey-ls marked this pull request as ready for review October 22, 2024 09:34
echedey-ls and others added 2 commits October 22, 2024 15:19
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@kandersolar kandersolar added this to the v0.11.2 milestone Oct 25, 2024
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

This looks great to me. I bet an example like this will be a good reference not just for pvlib, but other packages too, and even people's personal code.

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.

6 participants