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

Change PSM3 map_variables default value #1425

Closed
kandersolar opened this issue Mar 14, 2022 · 3 comments · Fixed by #2094
Closed

Change PSM3 map_variables default value #1425

kandersolar opened this issue Mar 14, 2022 · 3 comments · Fixed by #2094
Labels
Milestone

Comments

@kandersolar
Copy link
Member

#1374 did what we should probably be doing with most deprecations: mention the specific version when the deprecated behavior will change.

if map_variables is None:
warnings.warn(
'PSM3 variable names will be renamed to pvlib conventions by '
'default starting in pvlib 0.11.0. Specify map_variables=True '
'to enable that behavior now, or specify map_variables=False '
'to hide this warning.', pvlibDeprecationWarning)
map_variables = False

Tagging this in the 0.11.0 milestone so we don't forget it.

@kandersolar kandersolar added this to the v0.11.0 milestone Mar 14, 2022
@mikofski
Copy link
Member

mikofski commented Mar 15, 2022

isn't this part of some Sphinx directive somewhere? I always see markup like this that says...

deprecated since v0.X.Y

see: https://deprecated.readthedocs.io/en/latest/sphinx_deco.html

from deprecated.sphinx import deprecated


@deprecated(
    reason="""Since Python 3.4, you can use the standard function :func:`statistics.mean`.""",
    version="2.5.0",
)
def myfun(args):
    """
    Foobar.

    Blah.
    """
    return args

or this? https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-deprecated

Maybe this is only useful after the item is deprecated? IDK

@kandersolar
Copy link
Member Author

Well the pvlib._deprecation.deprecated has since and removal parameters and inserts that .. deprecated:: directive into the docstring too. But it inserts the directive at the top of the docstring so it applies to the entire function, not a single parameter.

I'm not sure I follow the suggestion though. Are you proposing we make a note in the docstring's description of map_variables that the default value will change in the future?

@mikofski
Copy link
Member

Sorry, I didn't think it through very well. I guess, this warning for map_variables seems like a maintenance burden, because we'll the deprecation version is hard-coded, so we need a follow up issue to update the warning message after v0.11 I think. It would be nice if there was a boilerplate way to have this somewhat automated, but probably not. It's not that big a deal

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 a pull request may close this issue.

2 participants