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

feat(typing): @deprecated versioning, IDE highlighting #3455

Merged
merged 15 commits into from
Jul 16, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 1, 2024

Supporting PR to attach to #3454

Notes

Deprecation should be handled using a decorator recognized by static type checkers:

  • @warnings.deprecated for sys.version_info >= (3, 13)
  • @typing_extensions.deprecated for earlier versions
  • Note: need to see how these handle subclassing subclassing seems to be unrecognized statically
    • Ideally want to have a version keyword-only, required argument
    • Indicating the altair==x.y.z this was introduced
  • Consistently use the same decorator to mark all deprecations
    • Updating existing warnings.warn & @utils.deprecation.deprecated
    • These should provide a reference for any edge cases, that required extending @typing_extensions.deprecated

Demo

The screenshot below shows the differences in behaviour of each decorator:

image

This is the ideal situation, where a type checker can identify the deprecation :

image

https://github.com/dangotbanned/altair/blob/fe1917e3d4986dfa431f428146819622b83b6546/altair/vegalite/v5/api.py#L796-L805

Console output

Sample code block
import typing_extensions as te
from altair.utils.deprecation import deprecated, AltairDeprecationWarning, deprecate
from altair.vegalite.v5.api import (
    selection,
    selection_point,
    selection_multi,
    selection_single,
)


@deprecated("", version="5.4.0", alternative="new_function")
def old_function(*args):
    return args


@te.deprecated("Deprecated in `altair=3.2.1`.", category=AltairDeprecationWarning)
def old_function_2(*args):
    return args


@deprecate(version="5.4.0", alternative="new_function")
def old_function_3(*args):
    return args


old_function()
old_function_2()
selection("point")
selection_multi()
old_function_3()
selection_point("not deprecated")

At runtime, Sample code block outputs:

Output
./../.py:26: AltairDeprecationWarning: alt.old_function was deprecated in `altair=5.4.0`. Use alt.new_function instead.
  old_function()
./../.py:27: AltairDeprecationWarning: Deprecated in `altair=3.2.1`.
  old_function_2()
./../.py:28: DeprecationWarning: Deprecated in `altair=5.0.0`. Use 'selection_point()' or 'selection_interval()' instead.

These functions also include more helpful docstrings.
  selection("point")
./../.py:29: AltairDeprecationWarning: alt.selection_multi was deprecated in `altair=5.0.0`. Use alt.selection_point instead.
  selection_multi()
./../.py:30: AltairDeprecationWarning: Deprecated in `altair=5.4.0`. Use new_function instead.
  old_function_3()

Parameter('not deprecated', SelectionParameter({
  name: 'not deprecated',
  select: PointSelectionConfig({
    type: 'point'
  })
}))

Summary

  • Use a single decorator, with the implementation of @deprecate but renaming to the prior @deprecated.
    • @deprecate and the screenshot demo are referencing fe1917e which also has a class implementation
  • Add an equivalent (in terms of parameters) warning version for deprecated code paths.
    • This wouldn't have the same static typing benefits, but would issue consistent messages and reduce repetition like in
      alt.param
      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")

Rejected Ideas

Applying @deprecated to individual overloads

This is a pretty interesting feature, but it comes with too many downsides.

Happy overload

image

Unhappy overload

image

Implementation

image

  • Only applicable to static type checkers, doesn't emit a runtime warning
    • Working around this would be possible, but not trivial
    • E.g. calling typing.get_overloads, writing some handler logic
  • Having the message displayed requires using the typing_extensions decorator
    • Which means manually maintaining consistent messages
    • Cannot enforce version included
  • Even with those conditions met, it still starts with The function "save" is deprecated ... - which is misleading
    • The deprecated part is the combination of arguments
    • In this case, the webdriver argument
  • It adds a lot of extra @overload code, which I don't think is warranted given the other issues

Supporting draft PR to attach to upcoming issue
This wrapper format seems to satisfy `pyright` enough to trigger a strikethrough
@dangotbanned dangotbanned changed the title feat(typing): DRAFT @deprecated improvements feat(typing): DRAFT @deprecated improvements Jul 1, 2024
@dangotbanned dangotbanned changed the title feat(typing): DRAFT @deprecated improvements feat(typing): @deprecated improvements Jul 3, 2024
@dangotbanned dangotbanned marked this pull request as ready for review July 3, 2024 18:26
@dangotbanned
Copy link
Member Author

@binste

This isn't a direct fix for #2947, but I think its an improvement for VSCode users.

image

@dangotbanned dangotbanned changed the title feat(typing): @deprecated improvements feat(typing): @deprecated versioning, IDE highlighting Jul 7, 2024
@binste binste self-requested a review July 15, 2024 18:18
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.

Nice one! :) Thanks for another great PR.

I only added 1 question. Also, I can't replicate that VS Code strikes through the deprecated functions:

image

I'm getting the deprecation warnings at runtime which is good but is there any way how I can check if pyright correctly identifies the deprecated function as being a wrapper around te.deprecated? And does mypy support this as well?

altair/vegalite/v5/api.py Outdated Show resolved Hide resolved
@dangotbanned
Copy link
Member Author

dangotbanned commented Jul 15, 2024

Nice one! :) Thanks for another great PR.

Thanks @binste

I'm getting the deprecation warnings at runtime which is good but is there any way how I can check if pyright correctly identifies the deprecated function as being a wrapper around te.deprecated? And does mypy support this as well?

In the top-level folder I've got a workspace (locally) with these settings:

Image

image

{
	"settings": {
		"python.analysis.diagnosticSeverityOverrides": {
            //"reportDeprecated": "warning",
            "reportInvalidTypeForm": "none",
            "reportMissingImports": "none",
            "reportMissingModuleSource": "none"
            //"reportUndefinedVariable": "none"
        },
		"python.analysis.inlayHints.functionReturnTypes": true,
    	"python.analysis.supportRestructuredText": true,
		"python.analysis.typeCheckingMode": "basic"
	}
}

I think "python.analysis.typeCheckingMode": "basic" is all that is needed for the strikethrough.

The actual diagnostic is "reportDeprecated", so you could try changing that.
When I was testing it, setting that option at all un-special-cases it into a regular warning/error/etc

Edit:
I'm pretty sure you need to be using the functions from outside the package for the visual part.

It works for me in UntitledTest1.ipynb


And does mypy support this as well?

This seems to be the tracking issue for it python/mypy#16111


No rush on the review @binste, just thought I'd be able to make that tweak quickly

Resolves vega#3455 (comment)

Note: The other imports changes are simply to align with my other PRs in review
@dangotbanned dangotbanned requested a review from binste July 15, 2024 20:31
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.

Thanks for the explanations and the link to the mypy PR! LGTM :)

@binste binste merged commit 9afd374 into vega:main Jul 16, 2024
11 checks passed
@dangotbanned dangotbanned deleted the deprecation-policy branch July 16, 2024 07:53
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 16, 2024
binste pushed a commit that referenced this pull request Jul 20, 2024
* refactor(typing): reduce type ignores in `api.py`

* feat(typing): adds `_is_undefined` guard

Direct copy from another PR, but is helpful here https://github.com/dangotbanned/altair/blob/d607c70824860ef3478d7d3996d26dc516d69369/altair/vegalite/v5/api.py#L504

* fix(typing): `ignore[index]` from #3455

* fix(typing): `[ignore[arg-type]` from #3452

* refactor(typing): use `_is_undefined` in some cases

* refactor: reduce code duplication in `Chart.to_dict`

* fix(typing): Recover `TopLevelMixin.resolve_` return types

I removed these in 6adf564 due to how complex they were getting, but both `mypy` and `pyright` seem happy with this solution

* fix(typing): narrow annotated types for `_check_if_valid_subspec`

Following investigation during #3480 (comment)

* refactor: remove unused `_wrapper_classes` argument from `SchemaBase`

Fixes:
- #3480 (comment)
- #3480 (comment)
- #3480 (comment)
@dangotbanned dangotbanned mentioned this pull request Jul 21, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants