-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[MAINT]: Decorator to warn about changed parameter names & allow for deprecation period #2237
[MAINT]: Decorator to warn about changed parameter names & allow for deprecation period #2237
Conversation
First thing is to consider if such a feature is needed/wanted for pvlib, feel free to weight in :) |
Seems elegant and useful, and easy to remove if it proves troublesome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am cautiously on board with the decorator :)
pvlib/solarposition.py
Outdated
times : :class:`pandas.DatetimeIndex` | ||
time : :class:`pandas.DatetimeIndex` | ||
Corresponding timestamps, must be localized to the timezone for the | ||
``longitude``. | ||
|
||
.. versionchanged:: 0.11.2 | ||
Renamed from ``times`` to ``time``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the docs somehow indicated that times
is still allowed, for now. Maybe we should we use deprecated
instead of versionchanged
during the deprecation period, and versionchanged
afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a point to state that it's still allowed. The less work the better so I vote for version changed only. Deprecating is already cumbersome and I'm hesitant to touch that require it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My line of thought was exactly the same of @AdamRJensen 's. However, I did hesitate about the message. It can be changed to something like:
.. versionchanged:: 0.12.0
Renamed from ``times`` to ``time``. ``times`` was deprecated in ``0.11.2``.
or
.. versionchanged:: 0.11.2
Renamed from ``times`` to ``time``. ``times`` will be removed in ``0.12.0``.
which explains both things but clutters the docs IMO.
I don't mind changing it thou.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's true that in v0.12.0 a follow-up PR to clean up the test that I have pushed now in 65e144b is required
Edit to complete the statement: a PR required that can also be used to clean up what is left and modify docs as needed.
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
I'd say this is now ready to choose a normalized deprecation/versionchanged directive format and discuss how to proceed in the future removal, or keep the admonitions. I have a slight preference for what's been done at |
Let me know if new actions are desired in this PR, or if you prefer to split add decorator / address issues in different PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is awesome!
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@echedey-ls I hate to ask for more work, but I think we should limit the scope of this PR to only creating the decorator (not applying it to real pvlib functions). Would you mind splitting out the solarposition.py
parameter renames into new PR(s)? That will give them better visibility.
It was helpful to see the decorator "in action" on real examples though :)
No problem. It doesn't make much sense to open the other PR now without the decorator on main, so I'll leave that up to you. These branch's commits already have the necessary changes to cherrypick them or just start from scratch, they aren't that many lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @echedey-ls!
docs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.@AdamRJensen is threatening my mail inbox with issues reports from the parameter renaming in #2231 and #2235 (just kidding). So maybe instead of making breaking changes we can have some deprecation period.
Maybe over-engineering a pattern in each function for each rename is not the best choice (my own idea), but we can work out some decorator to do so.
Here is that decorator, instructions and recommendations on how to use it:
pvlib.solarposition.hour_angle
, docs down below.Wiki?
In addition, I propose adding instructions on how to proceed with renamings in the wiki.
Docs
pvlib.solarposition.hour_angle
:pvlib.solarposition.sun_rise_set_transit_spa
: