Docstring cleanup: "default None", doi #1828

echedey-ls · 2023-08-04T11:44:53Z

Closes "default None" in docstring parameter descriptions considered harmful #1574 . All changes at the bottom.
I am familiar with the contributing guidelines
~~[ ] Tests added~~
~~[ ] Updates entries in docs/sphinx/source/reference for API changes.~~
[ ] 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`).
Current code includes numpydoc compliant docstrings, examples, and comments everywhere.
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.

Cherry-picked commits from #1790 and repeated the process with the regexes, with little changes to them.

Changes in this PR

Changes excessive use of parentheses in defaults
return_components: bool (optional, default=False) -> return_components : bool, default False
default None -> optional ("default None" in docstring parameter descriptions considered harmful #1574)
If None -> If not specified
Inexistent DOI roles are now roles, e. g.: DOI: 10... -> :doi:`10...`

Exclude .*=| from negative lookahead ^(\s{8}|\s{4})(?!try|else|doi|DOI|Warning|Access|Requests|Note)(\w*): (?!.*:)(.+)$ Reason: just this line 🌝 Remaining matches are type annotations in modelchain.py and pvsystem.py

F: (?<spaces>\s{8}|\s{4})(?<name>\w*)\s?: (?<type>.*) $optional, default=(?<default>.*)$$ R:

1st-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$ 2nd-F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn}one or (?<type>.*), optional$ 3rd-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$ R: $1$2 : $3, optional

F: If None, R: If not specified,

F: If None R: If not specified,

echedey-ls · 2023-09-30T17:15:45Z

regex DOI: ... to :doi:`...`

Done!

Regex find: (?:doi|DOI):(?!`)\s?(.*?)(\.\n|\n) Replace: :doi:`$1`$2

cwhanse

Thanks @echedey-ls

There's more markdown to be done in the descriptions of parameters this touches, if you feel it is in scope. I stopped suggesting it.

@pvlib/pvlib-maintainer it is questionable in my mind whether module_parameters, inverter_parameters and the like should be optional for instantiating a ModelChain. The ModelChain is created, true, but nothing runs.

pvlib/clearsky.py

pvlib/iotools/pvgis.py

pvlib/iotools/tmy.py

pvlib/modelchain.py

pvlib/pvsystem.py

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

…dey-ls/pvlib-python into refactor-docstring-params-2

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn]one[ ,]*(?<type>.*), optional$ R: $1$2 : $3, optional Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

F1: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*), default: None F2: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<types>.*), default None$ F3: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one or (?<type>.*), optional$ zero results F4: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*) or [nN]one, optional$ zero results R: $1$2 : $3, optional

cwhanse · 2023-10-31T14:41:14Z

@echedey-ls thanks for the work on this. If we don't get all instances of default None in this PR, we can do more later.

F: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*), default:? None\.? R: $1$2 : $3, optional F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one, (?<type>.*), default:? (?<def>.*)\.? R: $1$2 : $3, default $4 F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one or (?<types>.*) R: $1$2 : $3 F: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<types>.*) or [Nn]one(?<trailing>.*) R: $1$2 : $3$4 F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one(?:,|or) (?<types>.*) R: $1$2 : $3

echedey-ls · 2023-11-02T11:25:13Z

I had to iterate a bit more... At first glance I thought very constraining regexes were enough, but even trailing dots were messing with that. I think all have been addressed for now.

pvlib/iam.py

pvlib/iotools/pvgis.py

pvlib/irradiance.py

pvlib/location.py

pvlib/solarposition.py

This reverts commit b8f942b.

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

pvlib/irradiance.py

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

echedey-ls · 2023-11-03T21:36:55Z

This should be ready to go, unless anything else wants to be done!

kandersolar

LGTM, thanks @echedey-ls! Might be worth leaving a comment on this PR with a list of the final set of regexes you used for this PR, in case we want to re-run something like this in the future.

it is questionable in my mind whether module_parameters, inverter_parameters and the like should be optional for instantiating a ModelChain. The ModelChain is created, true, but nothing runs.

@cwhanse I'm not sure what you mean. ModelChain doesn't take module_parameters or inverter_parameters.

echedey-ls · 2023-11-06T18:37:12Z

None regexes

These go by order:
F: (?<spaces>\s{8}|\s{4})(?<name>\w*)\s?: (?<type>.*) \(optional, default=(?<default>.*)\)$
Manually fix every match. This is a very constraining regex.

F1: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*), default:? None\.?
F2: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one(?:,| or) (?<types>.*)
F3: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<type>.*) or [nN]one, optional
R: $1$2 : $3, optional
Note: F3 requires special attention to commas.

F: (?<spaces> {8}| {4})(?<name>\w*) ?: [Nn]one, (?<type>.*), default:? (?<def>.*)\.?
R: $1$2 : $3, default $4

F: (?<spaces> {8}| {4})(?<name>\w*) ?: (?<types>.*) or [Nn]one(?<trailing>.*)
R: $1$2 : $3$4

F: If None
R: If not specified,

DOI regexes:

Regex find: (?:doi|DOI):(?!`)\s?(.*?)(\.\n|\n)
Replace: :doi:`$1`$2

Some of them could be changed to be more flexible, but I wanted the least number of false positives. I'm looking at the excessive-parentheses regex, they are really ugly in the web.

kandersolar · 2023-12-12T15:51:48Z

With no objections, time to merge! Thanks again @echedey-ls :)

echedey-ls added 2 commits August 4, 2023 13:28

Align reference

29b3aa8

Update irradiance.py

15d8107

Exclude .*=| from negative lookahead ^(\s{8}|\s{4})(?!try|else|doi|DOI|Warning|Access|Requests|Note)(\w*): (?!.*:)(.+)$ Reason: just this line 🌝 Remaining matches are type annotations in modelchain.py and pvsystem.py

echedey-ls mentioned this pull request Aug 4, 2023

Refactor docstring params #1790

Closed

5 tasks

echedey-ls force-pushed the refactor-docstring-params-2 branch from 2623253 to 15d8107 Compare August 5, 2023 19:53

echedey-ls added 6 commits August 6, 2023 18:48

Remove parenthesis in types

9c5f6b2

F: (?<spaces>\s{8}|\s{4})(?<name>\w*)\s?: (?<type>.*) $optional, default=(?<default>.*)$$ R:

Remove none in type definition pvlib#1574

059f32c

1st-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<types>.*), default None$ 2nd-F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn}one or (?<type>.*), optional$ 3rd-F: (?<spaces> {8}| {4})(?<name>\w*) : (?<type>.*) or [nN]one, optional$ R: $1$2 : $3, optional

Change if none ocurrences (I)

96a4f58

F: If None, R: If not specified,

Change if none ocurrences (II)

a89addb

F: If None R: If not specified,

Too long line

e931a5d

Too long line

e9495e0

echedey-ls marked this pull request as ready for review August 6, 2023 20:52

echedey-ls added 2 commits October 28, 2023 21:04

fix doi external links

469edc2

Regex find: (?:doi|DOI):(?!`)\s?(.*?)(\.\n|\n) Replace: :doi:`$1`$2

Merge branch 'main' into refactor-docstring-params-2

7b3cc76

cwhanse reviewed Oct 30, 2023

View reviewed changes

echedey-ls and others added 12 commits October 31, 2023 12:27

Apply cwhanse suggestion

1bc3b12

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Apply cwhanse suggestion

57f67bb

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Apply cwhanse suggestion

8794a2e

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Apply cwhanse suggestion

2378db1

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Modified cwhanse suggestion

6171e77

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

Merge branch 'refactor-docstring-params-2' of https://github.com/eche…

3c3779e

…dey-ls/pvlib-python into refactor-docstring-params-2

Update modelchain.py

c75a662

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

Some None's missed

9c52070

F: (?<spaces> {8}| {4})(?<name>\w*) : [Nn]one[ ,]*(?<type>.*), optional$ R: $1$2 : $3, optional Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

none's, some more ``'s to variables and some other nones

232156c

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

Special none cases (please review)

b8f942b

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

Linter

f7c6c7c

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

cwhanse added enhancement documentation labels Oct 31, 2023

cwhanse added this to the v0.10.3 milestone Oct 31, 2023

echedey-ls added 2 commits November 1, 2023 13:03

Update irradiance.py

8ddcd7e

cwhanse approved these changes Nov 2, 2023

View reviewed changes

echedey-ls and others added 3 commits November 2, 2023 15:19

Revert "Special none cases (please review)"

1a39119

This reverts commit b8f942b.

Apply suggestions from code review

eb0806d

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Update irradiance.py

57d61af

cwhanse reviewed Nov 2, 2023

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

cwhanse reviewed Nov 2, 2023

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

echedey-ls and others added 2 commits November 2, 2023 23:17

Update pvlib/irradiance.py

69c8321

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Update irradiance.py

1f24c92

kandersolar approved these changes Nov 6, 2023

View reviewed changes

cwhanse mentioned this pull request Dec 8, 2023

v0.10.3 release plan #1928

Closed

15 tasks

AdamRJensen requested a review from kandersolar December 12, 2023 14:59

kandersolar changed the title ~~Refactor docstring params (2)~~ Docstring cleanup: "default None", doi Dec 12, 2023

kandersolar merged commit 300aedc into pvlib:main Dec 12, 2023
30 checks passed

echedey-ls deleted the refactor-docstring-params-2 branch December 12, 2023 16:57

echedey-ls mentioned this pull request Mar 5, 2024

Docs improvements tracker and ReadTheDocs warnings #1983

Open

5 tasks

echedey-ls mentioned this pull request Jun 10, 2024

No more default nones #2084

Merged

3 tasks

echedey-ls mentioned this pull request Jul 19, 2024

Add ape_e spectral factor model #2126

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docstring cleanup: "default None", doi #1828

Docstring cleanup: "default None", doi #1828

echedey-ls commented Aug 4, 2023 •

edited by cwhanse

Loading

echedey-ls commented Sep 30, 2023 •

edited

Loading

cwhanse left a comment

cwhanse commented Oct 31, 2023

echedey-ls commented Nov 2, 2023

echedey-ls commented Nov 3, 2023

kandersolar left a comment

echedey-ls commented Nov 6, 2023

kandersolar commented Dec 12, 2023

Docstring cleanup: "default None", doi #1828

Docstring cleanup: "default None", doi #1828

Conversation

echedey-ls commented Aug 4, 2023 • edited by cwhanse Loading

Changes in this PR

echedey-ls commented Sep 30, 2023 • edited Loading

cwhanse left a comment

Choose a reason for hiding this comment

cwhanse commented Oct 31, 2023

echedey-ls commented Nov 2, 2023

echedey-ls commented Nov 3, 2023

kandersolar left a comment

Choose a reason for hiding this comment

echedey-ls commented Nov 6, 2023

kandersolar commented Dec 12, 2023

echedey-ls commented Aug 4, 2023 •

edited by cwhanse

Loading

echedey-ls commented Sep 30, 2023 •

edited

Loading