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

docs: Versioning policy #3488

Merged
merged 22 commits into from
Aug 4, 2024
Merged

docs: Versioning policy #3488

merged 22 commits into from
Aug 4, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 20, 2024

Will close: #3454

Based on discussions in #3454 (comment) and #3454 (comment)

Public API definition is was blocking, see discussion in #3478 (reply in thread).

Tasks

@dangotbanned dangotbanned changed the title docs(DRAFT): versioning policy docs(DRAFT): Versioning policy Jul 21, 2024
@dangotbanned
Copy link
Member Author

Special handling of upstream vega-lite schema changes

I've moved this from the main task list, since I'm not sure of a good solution but maybe someone else has an idea.

  • Should altair support (w/ deprecation) input that is no longer compliant with the vega-lite schema?
  • See alt.param as of v5.0.0

parameter = Parameter(name)
if empty is not Undefined:
parameter.empty = empty
if parameter.empty == "none":
warnings.warn(
"""The value of 'empty' should be True or False.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
parameter.empty = False
elif parameter.empty == "all":
warnings.warn(
"""The value of 'empty' should be True or False.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
parameter.empty = True
elif (parameter.empty is False) or (parameter.empty is True):
pass
else:
msg = "The value of 'empty' should be True or False."
raise ValueError(msg)
if "init" in kwds:
warnings.warn(
"""Use 'value' instead of 'init'.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
if value is Undefined:
kwds["value"] = kwds.pop("init")
else:
# If both 'value' and 'init' are set, we ignore 'init'.
kwds.pop("init")

Issues

The example I've given highlights the conflict between how SemVer specifies deprecation and the most recent major version of altair.

v5.0.0 should be where a breaking change is made, but was instead a backwards-compatible change.

This is complicated by (AFAIK) the change occuring - without deprecation - in vega-lite.

@dangotbanned dangotbanned changed the title docs(DRAFT): Versioning policy docs: Versioning policy Jul 28, 2024
@dangotbanned dangotbanned marked this pull request as ready for review July 29, 2024 19:11
@dangotbanned
Copy link
Member Author

@mattijn Do you think there is a need to state here about the level of docs for deprecated functionality?
E.g. removal from API Reference, reduced docstrings

@mattijn
Copy link
Contributor

mattijn commented Jul 30, 2024

I trust you here👍

@binste
Copy link
Contributor

binste commented Jul 30, 2024

Special handling of upstream vega-lite schema changes

I've moved this from the main task list, since I'm not sure of a good solution but maybe someone else has an idea.

  • Should altair support (w/ deprecation) input that is no longer compliant with the vega-lite schema?
  • See alt.param as of v5.0.0

parameter = Parameter(name)
if empty is not Undefined:
parameter.empty = empty
if parameter.empty == "none":
warnings.warn(
"""The value of 'empty' should be True or False.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
parameter.empty = False
elif parameter.empty == "all":
warnings.warn(
"""The value of 'empty' should be True or False.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
parameter.empty = True
elif (parameter.empty is False) or (parameter.empty is True):
pass
else:
msg = "The value of 'empty' should be True or False."
raise ValueError(msg)
if "init" in kwds:
warnings.warn(
"""Use 'value' instead of 'init'.""",
utils.AltairDeprecationWarning,
stacklevel=1,
)
if value is Undefined:
kwds["value"] = kwds.pop("init")
else:
# If both 'value' and 'init' are set, we ignore 'init'.
kwds.pop("init")

Issues

The example I've given highlights the conflict between how SemVer specifies deprecation and the most recent major version of altair.

v5.0.0 should be where a breaking change is made, but was instead a backwards-compatible change.

This is complicated by (AFAIK) the change occuring - without deprecation - in vega-lite.

First thought is that if something is no longer compliant with the VL schema and it leads to a breaking change, we have to support it until the next major version following the policy you're drafting. If it's a feature which we know was heavily used by users and we can write some compatibility code around it, we can optionally consider deprecating it and still supporting it for a while. If we can make the lives of our users easier, I'm all for it :) What do you think?

@dangotbanned dangotbanned marked this pull request as draft July 31, 2024 09:23
@dangotbanned
Copy link
Member Author

First thought is that if something is no longer compliant with the VL schema and it leads to a breaking change, we have to support it until the next major version following the policy you're drafting. If it's a feature which we know was heavily used by users and we can write some compatibility code around it, we can optionally consider deprecating it and still supporting it for a while. If we can make the lives of our users easier, I'm all for it :) What do you think?

Thanks @binste, reading your comment and then re-reading the policy I've drafted has made me think I've overcomplicated this.
Will adapt that into the next draft

@dangotbanned dangotbanned marked this pull request as ready for review July 31, 2024 13:46
@dangotbanned
Copy link
Member Author

Ready for review @binste @mattijn

Feeling confident all the ideas that came up are now documented.

However, I'm not a writer, so please feel free to directly edit to taste

Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for me, thanks for putting it together.

@dangotbanned
Copy link
Member Author

Thanks both @joelostblom @mattijn

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precise and succinct, great!

@binste binste merged commit 95f654f into vega:main Aug 4, 2024
13 checks passed
@dangotbanned dangotbanned deleted the versioning-policy branch August 4, 2024 14:31
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.

Deprecation/breaking changes policy & process
4 participants