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

Fix AuthClient.create_policy signature #925

Closed
wants to merge 1 commit into from

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Dec 18, 2023

Fixing the signature is an inherently breaking change. Therefore, move to keyword-only arguments as a progressive upgrade which makes this easier to manage into the future.


Per our team discussion, although this is a violation of the semver backwards compatibility policy (and not in a marginal way, but very clearly we are breaking an API which we had previously declared), doing this does more good than harm.
The API is relatively newly released, and any fix here either has to encode these as optional positional args or else has to handle things in an awkward way.

Given the short lifetime to-date of the SDK version with this method available, we are making the decision that breaking the compatibility policy is better than strict adherence in this case.


📚 Documentation preview 📚: https://globus-sdk-python--925.org.readthedocs.build/en/925/

Fixing the signature is an inherently breaking change. Therefore, move
to keyword-only arguments as a progressive upgrade which makes this
easier to manage into the future.
@sirosen
Copy link
Member Author

sirosen commented Dec 18, 2023

Upon self-review, I do not feel comfortable with the backwards incompatible change.
Although we sometimes bend the rules on exactly what semver specifies (mostly around spacebar-heating-esque things), this is very clearly not allowed.

I'm closing this and will present a backwards compatible fix.

@sirosen sirosen closed this Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant