pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Change PSM3 map_variables default value

Open kandersolar opened this issue 3 years ago • 3 comments

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

https://github.com/pvlib/pvlib-python/blob/1893b20a7b755004f561037161c242db24e2870c/pvlib/iotools/psm3.py#L334-L340

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

kandersolar avatar Mar 14 '22 17:03 kandersolar

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

mikofski avatar Mar 15 '22 00:03 mikofski

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?

kandersolar avatar Mar 15 '22 14:03 kandersolar

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

mikofski avatar Mar 15 '22 21:03 mikofski