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

add typing information #475

Merged
merged 9 commits into from
Feb 13, 2023
Merged

add typing information #475

merged 9 commits into from
Feb 13, 2023

Conversation

ds-cbo
Copy link
Contributor

@ds-cbo ds-cbo commented Jan 25, 2023

add typing stubs to help IDE's and type checkers such as mypy and pyright

This deprecates the outdated https://github.com/ryanwang520/voluptuous-stubs

Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

I like the idea, but can't we just add the annotations to voluptuous directly? I'm okay with dropping support for older Pythons that don't support it.

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Jan 26, 2023

That's also fine. I didn't want to drop support just yet, but with your approval I'll merge them into the source :)

voluptuous/humanize.py Show resolved Hide resolved

Enum: typing.Union[type, None]
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, huh, I've never seen this syntax before, TIL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually you won't have to do this because the type can be inferred from context, but here it assumed it's always a type due to the import and then complains about the = None in the except ModuleError, so you'll have to be a bit more explicit :)

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Jan 27, 2023

I'm not quite sure why CI is failing:

   File "/opt/hostedtoolcache/Python/3.10.9/x64/lib/python3.10/site-packages/tox_setuptools_version/hooks.py", line 8, in <module>
    from tox.config import Config
ImportError: cannot import name 'Config' from 'tox.config' (/opt/hostedtoolcache/Python/3.10.9/x64/lib/python3.10/site-packages/tox/config/__init__.py)

@bluefish6
Copy link

I'm not quite sure why CI is failing:

  1. This repo uses tox-setuptools-version, which states:

This is a tox plugin that preinstalls a specific version of setuptools in each tox environment.

Note: This relies on an unstable tox plugin interface. You may experience breakage with new tox versions. If you do, please feel free to report the issue on Github.

seems like nobody reported the issue ;)

  1. It seems that tox-setuptools-version is not neccessary anymore, as setuptools pinning was already removed here: Extend README to include coverage run commands + unpin setupstools #465

  2. Please try to remove the Install tox-setuptools-version (lines 34-37) step of the test job and maybe it will start working :)

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Feb 9, 2023

@alecthomas Could you perhaps approve running the workflows to check whether that was indeed the fix?

@spacegaier
Copy link
Collaborator

@alecthomas Could you perhaps approve running the workflows to check whether that was indeed the fix?

Approved, but unfortunately there are still failures

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Feb 9, 2023

    def __init__(self, message: str, path: typing.List[str] | None = None, error_message: str | None = None, error_type: str | None = None) -> None:
TypeError: unsupported operand type(s) for |: '_GenericAlias' and 'NoneType'

Ah, I think the Type | None syntax is too new for versions below py-310. I'll rewrite them to typing.Optional[Type] soon, that should fix it

@spacegaier
Copy link
Collaborator

Getting there step-by-step :) . 2.7 is not yet happy and neither is flake8. 3.6 does not seem to exist as Github actions (anymore)?

@alecthomas
Copy link
Owner

Feel free to drop 2.7 if you like.

@bluefish6
Copy link

bluefish6 commented Feb 9, 2023

3.6 does not seem to exist as Github actions (anymore)?

Judging by the content of available python versions, it's still there, but only if you're running the job on ubuntu 20.04. However, as stated in the job log, ubuntu 22.04 is used. So either python 3.6 would need to be not tested anymore, or ubuntu-20.04 would need to be specified instead of ubuntu-latest in the workflow definition.

As Python 3.6 reached End Of Life at the end of 2021 (after 5 years from release, more than 1 year ago), I would like to suggest to the Voluptuous maintainers to consider dropping active support for it.

@alecthomas
Copy link
Owner

As Python 3.6 reached End Of Life at the end of 2021 (after 5 years from release, more than 1 year ago), I would like to suggest to the Voluptuous maintainers to consider dropping active support for it.

Definitely works for me!

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Feb 13, 2023

Should be another step closer :)

@alecthomas
Copy link
Owner

🤞

@alecthomas
Copy link
Owner

Success!

@alecthomas alecthomas merged commit bd2f9ad into alecthomas:master Feb 13, 2023
@alecthomas
Copy link
Owner

Lovely, thank you!

@ds-cbo ds-cbo deleted the generate-stub branch February 13, 2023 15:03
@@ -1148,7 +1159,7 @@ class Required(Marker):
{'key': []}
"""

def __init__(self, schema, msg=None, default=UNDEFINED, description=None):
def __init__(self, schema: dict, msg: typing.Optional[str] = None, default=UNDEFINED, description: typing.Optional[str] = None) -> None:
Copy link

Choose a reason for hiding this comment

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

Is this correct? Required can also take other scalar types e.g. the example in its docstring: schema = Schema({Required('key'): str})

Copy link
Owner

Choose a reason for hiding this comment

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

No I don't think it is. Want to send a PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I've overlooked that one. Commit coming up

@antoni-szych-rtbhouse
Copy link
Contributor

Hi @ds-cbo, I've updated the typings to allow integers in paths in #497 (useful to show that the validation failed for e.g. 5th element of some list), but wanted to confirm with you - was there any special reason you decided to allow only strings in path?

@ds-cbo
Copy link
Contributor Author

ds-cbo commented Dec 12, 2023

Hi @ds-cbo, I've updated the typings to allow integers in paths in #497 (useful to show that the validation failed for e.g. 5th element of some list), but wanted to confirm with you - was there any special reason you decided to allow only strings in path?

Answered in #497

@@ -7,6 +9,9 @@

import itertools
from voluptuous import error as er
from collections.abc import Generator
import typing
from voluptuous.error import Error
Copy link

Choose a reason for hiding this comment

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

Error could have been used as er.Error and this import would be not needed

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.

7 participants