-
Notifications
You must be signed in to change notification settings - Fork 57
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
add AsdfProvisionalAPIWarning #1295
add AsdfProvisionalAPIWarning #1295
Conversation
These changes taken from #1290 which is a candidate for using this warning. Other new features that are candidates for using this warning are:
|
Can we create a decorator so that we can just do something like: @asdf_provisional
def some_new_api(*args, **kwargs):
pass Instead of having to do all the raise warning stuff? |
A decorator that issues the warning might be nice. How would we handle #1289 where the change is to an existing method? |
I think a decorator like I am thinking of would look like: def asdf_provisional(func, msg=None):
msg = f"{func.__name__} has a provisional API, changes can happen at any time!" if msg is None else msg
def wrapper(*args, **kwargs):
warnings.warn(msg, AsdfProvisionalAPIWarning)
func(*args, **kwargs)
return wrapper |
I think we can model https://github.com/astropy/astropy/blob/e09ed4b9cddd48807564e498d3a21f16719c0e50/astropy/utils/decorators.py#L325 from |
Thanks for sending the astropy link. Do we need something that complex? Perhaps I'm not fully understanding what this warning will be used for. |
It doesn't need to be that complex. I was mostly thinking of adding it as a wrapper on new functions like I showed above. The astropy example is just something we could adapt (simplify) to meet our needs. |
I agree that something simpler than the linked astropy code is preferred. For the 3 PRs that could use this warning:
For #1287 this looks like a reasonable place for a warning: asdf/asdf/extension/_extension.py Lines 407 to 409 in 1ad66e3
we could call warnings.warn right before line 408 or adapt deprecated_attribute from astropy.
For #1289 we could call warnings.warn if Lines 871 to 872 in 802b759
or we could implement something like deprecated_renamed_argument (does this handle new arguments?)
For #1290 we could replace the generic warning with the ASDFProvisionalAPIWarning one: Lines 1856 to 1861 in 7e50d02
or use something like deprecated_attribute .
We might also consider using a provisional version of Are there other uses I'm missing? |
Thanks for the PR on the source branch for this PR: Looking at #1287 again the For #1289 we could decorate For #1290 it looks like for block_manager = provisional_attribute("block_manager") then assign to SerailzationContext._block_manager on init (to avoid issuing the warning every time). I got this wrong several times (thinking the provisional_attribute should decorate the property, etc) so is there a way to use this that I overlooked? With the above change, it would be possible for an extension to overwrite the block manager (using the As warnings.warn covers all our current uses my preference is to use it. An alternative to introducing a provisional api at all would be to just push development over to a 3.0 branch and add/change features without fear of breaking things. As the provisional api won't handle removal of features (like AsdfInFits in #1288) we will likely need to move to 3.0. I wouldn't object to shelving the provisional api and moving all the applicable changes to a 3.0 branch. |
The complexity if
It won't be able to cover all cases, such as this one.
I don't understand the point of decorating part of the private API like
The API for
This is fine, I prefer the decorator approach as it should amount to just adding an The other reason I prefer the decorator/convenience API here is that I think we might want tweak the All this being said feel free to reject my PR we just need to be consistent in however we construct the provisional API.
I strongly disagree with doing this. I personally think we should keep all this new API as provisional for at least one major release. Its fine to assume it doesn't get released until 3.0, but provisional status should not be removed until at least 4.0 in that case. The whole point of the provisional status was to enable us to release ASDF with the API so that we can use it and tweak it as we learn without worrying about backwards compatiblity concerns. As the API stabilizes we can remove the provisional status of components. |
Thanks for clarifying this. I did not understand that this was what you proposed during the asdf tag up. So that I'm understanding what you're proposing, are you saying that any new feature or api change we want to add:
|
Essentially, yes modulo how we decided to do versions (we may want to do 3.0 as just remove asdf-in-fits in which case we bump what you said by 1). I want all the experimental/new stuff to appear as provisional for a major release cycle and hopefully be finalized as part of the next major release. |
This warning should be used for any experimental api changes that should be considered provisional and might experience breaking changes (even in minor releases).
d958f8c
to
18e6496
Compare
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 just manually raising the warning is fine for now.
This warning should be used for any experimental api changes that should be considered provisional and might experience breaking changes (even in minor releases).