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

jsonschema: {relative,absolute}_path can hold ints #7980

Merged
merged 3 commits into from
May 29, 2022

Conversation

DMRobertson
Copy link
Contributor

In matrix-org/synapse#12901 I noticed an problem with the changes to the jsonschema stubs in #7950.

This change remedies that by teaching mypy that paths, relative paths and absolute paths on Error objects can hold strings, ints, or a mixture of the two. For evidence that this is the case, see e.g. https://github.com/python-jsonschema/jsonschema/blob/2e2832463e8de7977217d377ca0553c10f6fb48c/jsonschema/tests/test_exceptions.py#L246-L247 or the test case I provided in #7950 (comment).

@DMRobertson DMRobertson force-pushed the dmr/jsonschema-absolute-path branch from 15ea157 to 83a515b Compare May 29, 2022 14:47
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@DMRobertson DMRobertson marked this pull request as ready for review May 29, 2022 14:52
@AlexWaygood
Copy link
Member

@sirosen, does this change make sense to you? :)

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@sirosen
Copy link
Contributor

sirosen commented May 29, 2022

Yes, this change is definitely an improvement -- the OP's comment/example on the earlier PR is accurate, and I'm sorry I didn't catch this earlier.

I'm looking now to see if there are other [str] containers which should be [str | int]. But I'm not sure we want to block this while I do that?

@AlexWaygood
Copy link
Member

Yes, this change is definitely an improvement -- the OP's comment/example on the earlier PR is accurate, and I'm sorry I didn't catch this earlier.

I'm looking now to see if there are other [str] containers which should be [str | int]. But I'm not sure we want to block this while I do that?

I'll merge for now; further fixes can be made in further PRs if necessary.

Copy link
Member

@AlexWaygood AlexWaygood 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 fix, and thanks @sirosen for confirming!

@AlexWaygood AlexWaygood merged commit ea16964 into python:master May 29, 2022
DMRobertson pushed a commit to matrix-org/synapse that referenced this pull request May 29, 2022
Fixes #12901 by incoprating python/typeshed#7980

(The olddeps will automatically be fixed now, but let's pull in the more
complete stubs for day-to-day use while we're at it.)
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.

None yet

3 participants