Skip to content

Improve type annotations (add more and fix wrong ones)#1394

Merged
waylan merged 6 commits intoPython-Markdown:masterfrom
oprypin:annot
Oct 30, 2023
Merged

Improve type annotations (add more and fix wrong ones)#1394
waylan merged 6 commits intoPython-Markdown:masterfrom
oprypin:annot

Conversation

@oprypin
Copy link
Copy Markdown
Contributor

@oprypin oprypin commented Oct 25, 2023

The origins of these are three-fold:

  • Merging in stubs from https://github.com/python/typeshed/tree/main/stubs/Markdown using "merge-pyi"
    • Note: we can consider these annotations to be the important ones because it's what people have been adding according to their own need
  • Double-checking around places where stubs were already added from the above, particularly conflicts with annotations that got added in this repository already
    • Taking the opportunity to declare a generic "Registry of T" class
  • Running "mypy" and eliminating the most glaring errors it reported

@oprypin
Improve type annotations (add more and fix wrong ones)
81cb4bf 
The origins of these are three-fold:

* Merging in stubs from https://github.com/python/typeshed/tree/main/stubs/Markdown using "merge-pyi"
   - Note: we can consider these annotations to be the important ones because it's what people have been adding according to their own need
* Double-checking around places where stubs were already added from the above, particularly conflicts with annotations that got added in this repository already
   + Taking the opportunity to declare a generic "Registry of T" class
* Running mypy and eliminating the most glaring errors it reported
@waylan
Copy link
Copy Markdown
Member

waylan commented Oct 26, 2023

So there appears to be a couple code changes included in this (I don't mean adjustments to whitespace/line length). Could you please remove those? They should be submitted separately.

@oprypin
Copy link
Copy Markdown
Contributor Author

oprypin commented Oct 26, 2023

I only recognized one change that's clearly not about type annotations and reverted it.

I'm not sure if you also mean the NamedTuple change (which I think is on topic, as it's just the way to spell the same thing but with type annotations) or if there was something else.

waylan
waylan reviewed Oct 26, 2023
View reviewed changes
Comment thread markdown/util.py Outdated
@waylan
Copy link
Copy Markdown
Member

waylan commented Oct 26, 2023

I was thinking of the NameTuples, but you are correct, they do have type annotations. So nevermind on that.

oprypin
oprypin commented Oct 26, 2023
View reviewed changes
Comment thread markdown/util.py Outdated
Comment thread markdown/util.py Outdated
@waylan
Copy link
Copy Markdown
Member

waylan commented Oct 26, 2023

Thanks for this. Some of the wrong annotations I completely missed.

I'll merge this after a changelog entry is added.

@oprypin
Copy link
Copy Markdown
Contributor Author

oprypin commented Oct 27, 2023

That is done

@oprypin
Copy link
Copy Markdown
Contributor Author

oprypin commented Oct 27, 2023

For future reference:

  1. The main reason that nobody complained about the current type annotations is probably that they aren't actually used by type checkers. Unless a file named py.typed exists, the dominant type checker "mypy" ignores the types and asks you to install https://pypi.org/project/types-Markdown/ anyway.

  2. If you are interested in a fully correct type-checked codebase (according to "mypy") as well as official external-facing type annotations support (by adding py.typed), let me know anytime, I can take up that task and help maintain this going forward. Otherwise that's all for now.

@waylan
Copy link
Copy Markdown
Member

waylan commented Oct 30, 2023

Note that you have a conflict with the changelog (presumably with #1392).

Regarding a fully correct type-checked codebase (according to "mypy") that is probably a reasonable goal. Personally, I haven't really explored what's involved given that this is a rather new thing for Python. So contributions are welcome.

@oprypin
Copy link
Copy Markdown
Contributor Author

oprypin commented Oct 30, 2023 •
edited
Loading

Note that you have a conflict with the changelog

Hm? I had already merged to resolve the conflict. There was just the spelling check failing

Regarding a fully correct type-checked codebase (according to "mypy") that is probably a reasonable goal.

Great, so I'll continue looking into this soon (in a separate PR)

@waylan waylan merged commit 99425b4 into Python-Markdown:master Oct 30, 2023
@pawamoy
Copy link
Copy Markdown
Contributor

pawamoy commented Oct 30, 2023

This PR almost closes #1389 🙂

@oprypin
Copy link
Copy Markdown
Contributor Author

oprypin commented Oct 30, 2023

Oh I didn't see that!
No there are still like 100 mypy errors so maybe not "almost" 😅

@oprypin oprypin deleted the annot branch October 30, 2023 19:32
@pawamoy
Copy link
Copy Markdown
Contributor

pawamoy commented Oct 30, 2023

Oh OK I thought there was just the py.typed file missing 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@waylan waylan waylan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oprypin @waylan @pawamoy