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

Mark library as typed (PEP-561) #954

Merged
merged 1 commit into from May 23, 2022

Conversation

ssbarnea
Copy link
Contributor

@ssbarnea ssbarnea commented May 23, 2022

Avoids problems with mypy by officially declaring the library as typed.

Skipping analyzing "jsonschema": module is installed, but missing library stubs or py.typed marker

See: https://peps.python.org/pep-0561/

Avoids problems with mypy by officially declaring the library as
typed.

Skipping analyzing "jsonschema": module is installed, but missing library stubs or py.typed marker

See: https://peps.python.org/pep-0561/
@codecov-commenter
Copy link

Codecov Report

Merging #954 (0d20be2) into main (d715573) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #954   +/-   ##
=======================================
  Coverage   96.88%   96.88%           
=======================================
  Files          20       20           
  Lines        3276     3276           
  Branches      449      449           
=======================================
  Hits         3174     3174           
  Misses         79       79           
  Partials       23       23           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d715573...0d20be2. Read the comment docs.

@Julian
Copy link
Member

Julian commented May 23, 2022

Thanks.

@Julian Julian merged commit 77c7a30 into python-jsonschema:main May 23, 2022
@sirosen
Copy link
Member

sirosen commented May 24, 2022

Apologies for being late on this, but I saw this as the latest commit on main. Is this correct? jsonschema is in typeshed and does not include annotations for most of its own types.

I feel responsible as the person who introduced mypy and initial annotations to this project. I very intentionally -- out of respect for this project's current stance on typing -- did not take this measure or even start work towards it.

I believe, although I'm not certain, that this should be reverted.

@Julian
Copy link
Member

Julian commented May 24, 2022

Sorry! I misread the name of the person who approved this as yours, so that's me being sloppy. If this is incorrect I'll indeed revert!

@sirosen
Copy link
Member

sirosen commented May 24, 2022

I think reverting is for the best, if there haven't been more changes to support it. It's misleading at best and might take precedence over the typeshed stubs at worst.

I hope to gradually change your mind about typing and one day introduce this. However, part of that strategy of convincing you that the typing stuff is valuable is to not cause disruption here.

@Julian
Copy link
Member

Julian commented May 24, 2022

Good strategy :)

Sounds good thanks for the advice! Definitely open to gradually getting there.

Julian added a commit that referenced this pull request May 24, 2022
This reverts commit 77c7a30, reversing
changes made to d715573.

See #954 (comment)
@Julian
Copy link
Member

Julian commented May 24, 2022

Done. Thanks for catching!

@ssbarnea
Copy link
Contributor Author

I am even more confused, I was not expecting why having the lib not typed is desired. Typeshed is more of historical hack that does not scale and it is always outdated.

@Julian
Copy link
Member

Julian commented May 24, 2022

It's just not something that's been high priority. There are other more important issues to work on in the queue. Small self-contained patches are welcome.

@ssbarnea
Copy link
Contributor Author

I could try to make few such patches to help with this. Adding type is tricky and hard to prioritise, a gradual approach worked for the projects I work on proved to work decently. There are even typing challenges around JSON arguments and return types which are not yet addressed.

There were some amazing and reactively unexpected benefits that I found since I started to use types everywhere, forced a much better design when writing code.

@sirosen
Copy link
Member

sirosen commented May 24, 2022

I was not expecting why having the lib not typed is desired. Typeshed is more of historical hack that does not scale and it is always outdated.

In an issue discussion I recall Julian being wary of taking on extra burdens in jsonschema in order to support type annotations. Small and incremental, as you have suggested, is definitely the way to go.

I agree that typeshed is a temporary workaround, not a long-term solution. I only mention it because that is the current location for type annotations for jsonschema, so we need to be aware of that.

[using] types everywhere, forced a much better design when writing code.

I've found this as well, but I've also found that idiomatic or even very good designs can conflict with the limitations of type checkers.

@Julian
Copy link
Member

Julian commented May 24, 2022

Sounds like we're all mostly in agreement :) if/when it turns out type checking reveals either a bug or a better design, that certainly will help me come around -- until then I view it basically as a documentation improvement, and yeah there are lots of docs improvements I want to make, this being just one of em. But yeah I appreciate both of your help!

Julian added a commit to sirosen/jsonschema that referenced this pull request Jun 2, 2022
* main:
  Slightly speed up pip installs by skipping the version check in CI.
  Remove the now-unused MANIFEST.in.
  v4.6.0 -> CHANGELOG
  Ignore the badge URLs, they seem super unreliable from CI.
  Ignore a deprecation warning coming from pkg_resources on 3.11
  Add basic CONTRIBUTING guidelines.
  Make project.urls be valid URLs.
  Combine the CI and packaging workflows.
  Let RTD be authoritative about what the default doc version is.
  Re-enable Python 3.11 testing in CI.
  Modernize the packaging setup via PEP 621 and Hatch.
  Update various GHA versions.
  Update docs requirements.
  Revert "Merge pull request python-jsonschema#954 from ssbarnea/fix/py.typed"
@ssbarnea
Copy link
Contributor Author

ssbarnea commented Jun 7, 2022

@Julian Maybe we can find a way to address this?

Apparently 4.6.0 already broke integration because some types were changed and the types-jsonschema is heavily outdated:

    ERROR: Could not find a version that satisfies the requirement types-jsonschema>=4.6.0 (from versions: 3.2.0, 3.2.1, 4.2.0, 4.3.0, 4.3.1, 4.3.2, 4.4.0, 4.4.1, 4.4.2, 4.4.3, 4.4.4, 4.4.5, 4.4.6, 4.4.7, 4.4.8, 4.4.9)

Basically I cannot run mypy on my code with jsonschema 4.6.0 because there are no types for it and the change to add them was reverted.

Personally I do not think that maintaining types separately is sustainable at all. Maybe it was ok 2-3 years ago when types were not so well spread and for libraries that were not under active maintenance. Still for any software that is actively maintained, like this library it worth not offloading the types to someone else.

The errors that I seen were related to a type mismatch for jsonschema.ValidationError.absolute_path which looked like:

 has incompatible type "deque[Union[str, int]]"; expected "deque[str]"

When I went to the source code, I seen a method without any types on it. To guess which types were involved it would require a lot of digging and grepping around the source code.

@Julian
Copy link
Member

Julian commented Jun 7, 2022

the change to add them was reverted.

As I understand it (as it was explained to me really), the change here was just purely incorrect, it marks this library as shipping types when it does not yet. Again, such PRs are welcome in small chunks.

@ssbarnea
Copy link
Contributor Author

ssbarnea commented Jun 7, 2022

Super. I will include the stub files from typeshed and that should make the PR valid. We can wait for others to check it before merging as we don't want to break it.

@sirosen
Copy link
Member

sirosen commented Jun 7, 2022

I have been working with the typeshed maintainers to improve the stubs, and I have been working on finding good candidates to add to mypy_primer.

The stubs are not badly out of date, and I'm not sure what gives you the impression that they are. I just got a PR merged to update them in the last week or two, and I followed up here by starting to get the format checker code annotated.

There is an incremental path I'm following here to get the stubs gradually more fleshed out, leveraging the expertise of the typeshed team and the infrastructure they have with mypy_primer and other CI. As we expand the stubs, I'll be working here to backport the annotations and resolve any inconsistencies and failures.

Please do not add a copy of the stubs here. It will only get in the way of the active effort to add annotations.

@ssbarnea
Copy link
Contributor Author

ssbarnea commented Jun 7, 2022

@sirosen I am quite curious how would you do a progressive/stepped switch without this approach because adding any type info to the library itself would not be testable by users once they are already forced to use typeshed and the lack of py.typed marked prevents them to use the new types.

I see this as a chicken and the egg problem and IMHO doing a copy from typeshed allows us to migrate these into code gradually, after this move, like one file at a time.

@sirosen
Copy link
Member

sirosen commented Jun 7, 2022

The goal is to develop fully fleshed out stubs in typeshed, backporting those annotations to jsonschema as we go. The approach was laid out here: python/typeshed#7950 (comment)

There were a small number of errors in those annotations. I regret that, but they were found and fixed quickly because the stubs are actively used. These PRs fixed issues:
python/typeshed#7980
python/typeshed#7981
python/typeshed#7990

The typeshed owners also made this adjustment:
python/typeshed#8024

I also added check-jsonschema to mypy_primer:
hauntsaninja/mypy_primer#35

If there are other libraries which should be added to mypy_primer, that would be most welcome. Note that mypy_primer does not support the use of mypy plugins at present.

The TypeAlias change is a good demonstration that there is expertise in typeshed which is worth leveraging. typeshed also has some capabilities which are not broadly available in the _typeshed imports, which allows them to run ahead of what is provided in typing_extensions and mypy with things like their custom Self and Comparable types.

@arthurio
Copy link

@Julian Would you mind removing Mark library as typed (PEP-561) by @ssbarnea in https://github.com/python-jsonschema/jsonschema/pull/954 from the release description? It's a bit confusing since it has been reverted 🙃

@Julian
Copy link
Member

Julian commented Jun 21, 2022

Done!

@arthurio
Copy link

That was fast 🚀 ! Thank you 😃

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

6 participants