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

Is types-jsonschema outdated? #8662

Open
kratsg opened this issue Sep 1, 2022 · 15 comments
Open

Is types-jsonschema outdated? #8662

kratsg opened this issue Sep 1, 2022 · 15 comments
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: false positive Type checkers report false errors

Comments

@kratsg
Copy link

kratsg commented Sep 1, 2022

ref: python-jsonschema/jsonschema#997

I expected the following code to work when it comes to typing

from typing import Any, Mapping
import jsonschema

store: dict[str, Mapping[str, Any]] = {}

jsonschema.RefResolver(
    base_uri=f"file://mypath.json",
    referrer={},
    store=store
)

However, this gives an error:

test.py:9: error: Argument "store" to "RefResolver" has incompatible type "Dict[str, Mapping[str, Any]]"; expected "Union[SupportsKeysAndGetItem[str, str], Iterable[Tuple[str, str]]]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

defined at

class RefResolver:
referrer: dict[str, Any]
cache_remote: Any
handlers: dict[str, _Handler]
store: URIDict
def __init__(
self,
base_uri: str,
referrer: dict[str, Any],
store: SupportsKeysAndGetItem[str, str] | Iterable[tuple[str, str]] = ...,
cache_remote: bool = ...,
handlers: SupportsKeysAndGetItem[str, _Handler] | Iterable[tuple[str, _Handler]] = ...,
urljoin_cache: Any | None = ...,
remote_cache: Any | None = ...,
) -> None: ...
. It seems incorrect that store is a dict[str, str] type.

Similarly, referrer is defined as a dict (MutableMapping) but I suspect that could be changed to Mapping (generically).

@hauntsaninja
Copy link
Collaborator

Yeah, the type for store definitely looks wrong. PR welcome!

@AlexWaygood AlexWaygood added the help wanted An actionable problem of low to medium complexity where a PR would be very welcome label Sep 2, 2022
@AlexWaygood
Copy link
Member

Cc. @sirosen, for interest :)

@srittau srittau added stubs: false positive Type checkers report false errors size-small labels Sep 2, 2022
@sirosen
Copy link
Contributor

sirosen commented Sep 11, 2022

I agree -- these seem like minor mistakes (and I bet git blame shows my name! 😅 ). I'll try to work on this within the next couple of days.

For changing anything from dict or MutableMapping to Mapping, I might want to get that changed in jsonschema itself, first. That way, the type-checking on the library will match the contract being declared by the stubs.

(I'm just getting back to various things from a short break; thanks for the ping!)

@DanielNoord
Copy link

There also seem to be some issues with ValidationError. Specifically ValidationError.validator is typed as being Validator | None while it should be str | jsonschema._utils.Unset.

@AlexWaygood
Copy link
Member

PR welcome :)

@DanielNoord
Copy link

@AlexWaygood I'll try and find some time for this somewhere this weekend. What's the policy for version differences and using internal private modules?

@AlexWaygood
Copy link
Member

@AlexWaygood I'll try and find some time for this somewhere this weekend.

Great, thank you!

What's the policy for version differences...

Typeshed only ever has stubs for the latest version of the runtime; we currently don't have the tooling to allow us to do anything else. (Even if we could, it would be something of a maintenance burden to do so...)

...and using internal private modules?

We generally don't like to included internal private modules "for completeness' sake". But if a public module imports something from a private module, it's sometimes preferable to stay close to the implementation and include the private module in the stub. This varies from case to case somewhat tbh.

@DanielNoord
Copy link

DanielNoord commented Nov 25, 2022

In this case jsonschema seems to use a minimal class to represent attributes not being set:

class A():
    def __init__(attr: str | Unset = Unset):
        self.attr = attr

With Unset coming from a private _utils module. Should that then be used as well?

@AlexWaygood
Copy link
Member

It looks like we already have _utils.Unset in the stub:

So yeah, I'd just import it from _utils.pyi, mimicking the runtime exactly :)

@DanielNoord
Copy link

I found that the maintainer of jsonschema is actually open to adding annotations to the project itself. I'll see how quickly that process goes. If there is a reasonable chance to include some updated typing into the project itself I guess it is better to do so and not duplicate efforts with updating typeshed as well.

I'll update once I know more. Thanks for the explanation anyway Alex!

@AlexWaygood
Copy link
Member

Yes, @sirosen has been quite involved with the effort to add type hints to jsonschema! I believe the plan is to develop typeshed's stubs and the upstream annotations in tandem, until jsonschema is ready to add a py.typed file. But @sirosen can probably tell you more (this was described a little bit in the PR description for #7950).

@DanielNoord
Copy link

Hmm, the reason why I initially got interested in these stubs is because the stubs added there are incorrect. Most importantly the _Error.validator attribute which is messing with some of our company's internal type checking.
That said I have spotted an issue with my PR (python-jsonschema/jsonschema#1019) based on the PR by @sirosen so I'll amend that.

I'd happily cooperate with adding the stubs to typeshed as well for the time being! Although it feels like if the project is open to receiving annotations themselves it's better to merge them there as there is less risk of decoupling.

@sirosen
Copy link
Contributor

sirosen commented Nov 26, 2022

I just wanted to chime in to say I am still interested in working on fixing up these stubs, according to the plan which I pitched. I've been juggling various projects and haven't prioritized this one, but I'll make an effort to change that.

@DanielNoord
Copy link

@sirosen What's your reason for doing both an effort in typeshed and in jsonschema? Isn't it more worthwhile and efficient to do this in one place? Getting review from the jsonschema maintainers in the actual repository would probably also prevent more errors from slipping into the stubs.

@kratsg
Copy link
Author

kratsg commented Nov 27, 2022

@sirosen I just wanted to say thanks, but I hope it doesn't sound like I'm demanding for anything from you. I am still somewhat new to python typing, so I opened this issue because I will assume that other people (esp in typeshed) write better typed code than I can, but it's good to know that there's effort here. It wasn't clear to me that this was happening in jsonschema directly, but I only found it in here. [If I had more time... I know I know...]

Do you have a publicly documented roadmap describing this in jsonschema?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: false positive Type checkers report false errors
Projects
None yet
Development

No branches or pull requests

6 participants