Skip to content

removed kwargs in calcparams_desoto, calcparams_cec, and sapm #1222

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

Merged
merged 5 commits into from
May 12, 2021

Conversation

MichaelHopwood
Copy link
Contributor

@MichaelHopwood MichaelHopwood commented May 7, 2021

Removed the **kwargs parameters from calcparams_cec, calcparams_desoto, and sapm. Python automatically returns a TypeError upon user input of unexpected parameters.

  • Closes Remove shadowed/unused kwargs from PVSystem methods #1118
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse cwhanse mentioned this pull request May 10, 2021
24 tasks
@cwhanse cwhanse added the api label May 11, 2021
@cwhanse cwhanse added this to the 0.9.0 milestone May 11, 2021
@wholmgren
Copy link
Member

Do we want to remove these kwargs without deprecation? I am not opposed since I don't think it would hit many people and it would most likely be in code that can be fixed relatively easily.

Depending on the decision about deprecation, the note in the whats new needs to move to breaking changes or to deprecations.

@cwhanse
Copy link
Member

cwhanse commented May 11, 2021

I'm ok without deprecation, I doubt that kwargs was used much for these PVSystem methods. Agree with @wholmgren about the placement of the whatsnew note.

@MichaelHopwood
Copy link
Contributor Author

Thanks @wholmgren and @cwhanse for the feedback. I moved the whatsnew note to the breaking changes section.

@MichaelHopwood
Copy link
Contributor Author

Thanks @wholmgren and @kanderso-nrel for the feedback! I updated the whatsnew file per your suggestions.

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.

LGTM thanks @MichaelHopwood!

@wholmgren
Copy link
Member

thanks @MichaelHopwood!

@wholmgren wholmgren merged commit 0415365 into pvlib:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove shadowed/unused kwargs from PVSystem methods
4 participants