Open kratsg opened 2 years ago
Yeah, the type for store
definitely looks wrong. PR welcome!
Cc. @sirosen, for interest :)
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!)
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
.
PR welcome :)
@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 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.
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?
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 :)
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!
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).
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 (https://github.com/python-jsonschema/jsonschema/pull/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.
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.
@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.
@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
?
ref: python-jsonschema/jsonschema#997
I expected the following code to work when it comes to typing
However, this gives an error:
defined at https://github.com/python/typeshed/blob/c1d307fc3bf51488b708e1c71334faccba5fd294/stubs/jsonschema/jsonschema/validators.pyi#L66-L80 . It seems incorrect that
store
is adict[str, str]
type.Similarly,
referrer
is defined as adict
(MutableMapping
) but I suspect that could be changed toMapping
(generically).