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.

Sign up for GitHub

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

Jump to bottom

feat: Adds `vl_convert.pyi` type stub #185

Merged

jonmmease merged 8 commits into main from py-manual-stubs

Aug 31, 2024

Member

dangotbanned commented Aug 28, 2024 •

edited

Loading

Closes #184

Updated

Tried enabling pyright on the tests/ directory and answered my own question on Option.
With that fixed, this is an example in VSCode:

Suggestion

You could setup mypy to run on tests/ to:

Confirm whatever you've expected in a test will not raise a warning for users
Notify if stubs become out of sync with the actual interface


          feat: Adds vl_convert.pyi type stub

786beda

#184

dangotbanned commented

View reviewed changes

vl-convert-python/pyproject.toml Outdated

Comment on lines 17 to 78

+              [tool.ruff]
+              target-version = "py310"
+              line-length = 88
+              indent-width = 4
+              exclude = []
+              [tool.ruff.format]
+              quote-style = "double"
+              indent-style = "space"
+              skip-magic-trailing-comma = true
+              line-ending = "lf"
+              # https://docs.astral.sh/ruff/formatter/#docstring-formatting
+              docstring-code-format = true
+              docstring-code-line-length = 88
+              [tool.ruff.lint]
+              # https://docs.astral.sh/ruff/preview/
+              preview = true
+              # https://docs.astral.sh/ruff/settings/#lint_extend-safe-fixes
+              extend-safe-fixes = [
+                  # from __future__ import annotations #
+                  # ---------------------------------- #
+                  "UP006",
+                  "UP007",
+                  "UP008",
+                  "TCH",
+                  # unsorted-dunder-all
+                  "RUF022",
+                  # pydocstyle #
+                  # ---------- #
+                  # fits-on-one-line
+                  "D200",
+                  # escape-sequence-in-docstring
+                  "D301",
+                  # ends-in-period
+                  "D400",
+              ]
+              extend-select = [
+                  "ANN",
+                  "D",
+                  "D213",
+                  "D400",
+                  "E",
+                  "F",
+                  "FA",
+                  "I001",
+                  "RUF",
+                  "TCH",
+                  "TID",
+                  "UP",
+                  "W",
+              ]
+              ignore = [
+                  # indent-with-spaces
+                  "D206",
+                  # multi-line-summary-first-line ((D213) is the opposite of this)
+                  "D212",
+                  # Line too long
+                  "E501",
+              ]
+              pydocstyle.convention = "numpy"

Member Author

dangotbanned Aug 28, 2024

I added a subset of https://github.com/vega/altair/blob/main/pyproject.toml to try and keep a consistent style with altair.

AFAIK these only apply locally, since vl-convert is using black.

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi

Comment on lines +104 to +119

+                  VegaThemes: TypeAlias = Literal[
+                      "carbong10",
+                      "carbong100",
+                      "carbong90",
+                      "carbonwhite",
+                      "dark",
+                      "excel",
+                      "fivethirtyeight",
+                      "ggplot2",
+                      "googlecharts",
+                      "latimes",
+                      "powerbi",
+                      "quartz",
+                      "urbaninstitute",
+                      "vox",
+                  ]

Member Author

dangotbanned Aug 28, 2024

Lifted direct from the result of vega/altair#3523

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi

+              if TYPE_CHECKING:
+                  from typing import Any, Literal, TypeAlias
+                  FormatLocaleName: TypeAlias = Literal[

Member Author

dangotbanned Aug 28, 2024

FormatLocaleName & TimeFormatLocaleName I got by following the links that you had in the docs.

The repos seem to have remained pretty constant, so maintaining these (hypothetically) would be on the scale of 2-7 years between changes.

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi

+                  Renderer: TypeAlias = Literal["canvas", "hybrid", "svg"]
+                  FormatLocale: TypeAlias = FormatLocaleName | dict[str, Any]
+                  TimeFormatLocale: TypeAlias = TimeFormatLocaleName | dict[str, Any]
+                  VlSpec: TypeAlias = str | dict[str, Any]

Member Author

dangotbanned Aug 28, 2024 •

edited

Loading

Though this alias (VlSpec) could be a placeholder for if you decided to get more specific in the future.

E.g. if you expect a certain level of nesting, this can be updated and propagate to all relevent defs.

The screenshots in vega/altair#3536 show examples of the utility of this

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi Outdated

Comment on lines 441 to 455

+                  Parameters
+                  ----------
+                  vg_spec
+                      Vega JSON specification string or dict
+                  allowed_base_urls
+                      List of allowed base URLs for external data requests.
+                      Default allows any base URL
+                  format_locale
+                      d3-format locale name or dictionary
+                  time_format_locale
+                      d3-time-format locale name or dictionary
+                  Returns
+                  -------
+                  scenegraph.

Member Author

dangotbanned Aug 28, 2024 •

edited

Loading

This applies to all docstrings, but I've omited the types since we have them in the signature.

Also most of the descriptions repeat the same information - but I've kept that in

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi Outdated

+                  renderer: Renderer | None,
+              ) -> str:
+                  """
+                  Convert a Vega-Lite spec to self-contained HTML document using a particular version of the Vega-Lite JavaScript library.

Member Author

dangotbanned Aug 28, 2024 •

edited

Loading

I had to disable line-too-long for docs like this.

Splitting it into 2 lines broke multi-line-summary-second-line.

Just thought I'd note this with links to the rules, not proposing any changes

dangotbanned commented

View reviewed changes

vl-convert-python/vl_convert.pyi Outdated Show resolved Hide resolved

dangotbanned requested a review from jonmmease

August 28, 2024 12:31

dangotbanned and others added 3 commits

August 31, 2024 12:38


          fix(typing): Add default values for Option types

69a0549

#185 (comment)


          ci(ruff): Only lint/format type stub

3f96c10


          Updates for Python 3.8 compat and a couple of docstring / signature u…

c3338c7

…pdates

Collaborator

jonmmease commented Aug 31, 2024

Hi @dangotbanned, thank for working on this. I checked out the branch to review and updated the type signatures for Python 3.8+ compatibility, since vl-convert is a bit more conservative than vega-altair (I try to use current vl-convert with prior versions of Vega-Altair).

Take a look and see if that makes sense to you, then I'll merge it

Member Author

dangotbanned commented Aug 31, 2024 •

edited

Loading

Hi @dangotbanned, thank for working on this. I checked out the branch to review and updated the type signatures for Python 3.8+ compatibility, since vl-convert is a bit more conservative than vega-altair (I try to use current vl-convert with prior versions of Vega-Altair).

Take a look and see if that makes sense to you, then I'll merge it

Thanks @jonmmease!

On the 3.8 compatibility, did the stub cause any runtime issues for you?
If not, I would suggest reverting those changes since AFAIK .pyi files are allowed to use features from newer versions https://typing.readthedocs.io/en/latest/spec/distributing.html#stub-files

Edit

Looking at the jsonschema stubs - I think we only need the from typing_extensions import TypeAlias

dangotbanned added 4 commits

August 31, 2024 13:42


          ci: Target python 3.8

934429e


          fix: Apply subset of c3338c7

4b8adbb


          Merge branch 'py-manual-stubs' of https://github.com/vega/vl-convert …

a85cd09

…into py-manual-stubs


          revert: Fix conflicts introduced by fix: Apply subset of c3338c7

300323e

Member Author

dangotbanned commented Aug 31, 2024 •

edited

Loading

@jonmmease I'm thinking this is unrelated to that series of changes?

https://github.com/vega/vl-convert/actions/runs/10645388913/job/29511135603?pr=185

Maybe related but I absolutely botched the merge and had to overwrite the whole file with what I had in 4b8adbb (#185)

Collaborator

jonmmease commented Aug 31, 2024

Yes, that CI failure is unrelated. There seems to be an inconsistency in the files that are pulled down from skypack, where they sometimes include a trailing comment. I've been meaning to update the vendoring logic to strip out trailing comments for this reason.

I'll restart CI and see if that resolves it for now.

On the 3.8 compatibility, did the stub cause any runtime issues for you?
If not, I would suggest reverting those changes since AFAIK .pyi files are allowed to use features from newer versions

Oh, you're right, these files are only for mypy 🤦‍♂️ Looks like you're merge reverted to the original form, so that works for me.

Collaborator

jonmmease commented Aug 31, 2024

It passes now and looks good to me. Are you ready for it to merge?

Member Author

dangotbanned commented Aug 31, 2024

It passes now and looks good to me. Are you ready for it to merge?

Perfect, yeah I'm happy for you to merge

jonmmease merged commit 8f5200c into main

10 checks passed

jonmmease deleted the py-manual-stubs branch

August 31, 2024 14:41

dangotbanned mentioned this pull request

Improve contributing/dev setup guide #186

Closed

dangotbanned added a commit to vega/altair that referenced this pull request


          fix(typing): Add patch for vl-convert-python=1.7.0

aa00dc1

Will open a new PR to merge this, related to vega/vl-convert#185

dangotbanned added a commit to vega/altair that referenced this pull request


          ci: Bump vl-convert-python to 1.7.0

c46cbde

Related:
- aa00dc1
- vega/vl-convert#185
- vega/vl-convert#191

This was referenced Oct 6, 2024

ci: Bump vl-convert-python to 1.7.0 vega/altair#3633

Merged

fix(typing): Add untyped vegafusion to mypy overrides vega/altair#3640

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet