Python-Markdown / markdown

A Python implementation of John Gruber’s Markdown with Extension support.
https://python-markdown.github.io/
BSD 3-Clause "New" or "Revised" License
3.74k stars 858 forks source link

Fix all mypy typechecking errors, add a lot of type annotations #1399

Closed oprypin closed 10 months ago

oprypin commented 10 months ago
oprypin commented 10 months ago

See my comments below and please make the adjustments everywhere, not just where I've noted them.

The adjustments would basically mean reverting the changes, but almost none of them can be reverted, because then mypy errors appear.

So this would just mean giving up trying to check the code with mypy. Maybe there is some misconception, but the point of running the actual mypy checks is that it also checks function bodies and can complain about potential problems until you write code in a way that makes it clear to mypy that there is no problem.

oprypin commented 10 months ago
waylan commented 10 months ago

Maybe there is some misconception

Python not being strongly typed if a feature which I embrace. I will continue to write my code that way. Anything which forces me to write strongly typed code in a language which is not strongly typed is unwelcome.

I see type annotations as documentation. The code dictates the annotations, not the other way around. Of course, over time as updates and changes are made to the code, the annotations could get out-of-sync with the code. Therefore, it is reasonable to include a type checker in the CI to ensure that doesn't happen. However, the type checker should only be checking the already defined annotations. There is no need to type check an unannotated private variable which is never used outside of a single method (for example).

If mypy cannot serve that purpose, then it is the wrong tool for the job.

waylan commented 10 months ago

I am going to close this in favor of #1401. I suspect that mypy is not the right fit for this project. However, if it can be configured to fit, then this can be reopened. Or, if a different tool is proposed, that should be in a new PR.

oprypin commented 10 months ago

I'll want to sync this branch to latest if #1401 is merged, and then leave this for posterity. Not sure if I'll be able to do that if it's closed.

oprypin commented 10 months ago

Yeah mypy is pretty bad! Sadly other checkers would ask for even more code changes :D

waylan commented 10 months ago

Okay, I'll leave it open for now.

oprypin commented 10 months ago

By disabling a large amount of mypy checks, I reduced this to a much smaller change #1402 that is based on #1401. It will ensure the type annotations don't fall into disrepair.

facelessuser commented 10 months ago

I think if you are using types, using mypy is very important to keep things from getting broken. I also agree that there is nothing wrong with limiting the checks and/or doing per line ignores for particularly tricky things that can be resolved later if there is interest.

waylan commented 10 months ago

I have taken a step back and taken a fresh look at the overall matter of annotations and type checking. I think perhaps I dove in with a few misunderstandings and miscomprehensions. Sorry about that; especially if I have led anyone down a path of doing work I would never approve. That was not my intention.

So to be clear. upon reflection, the following are my general expectations.

  1. This library should always be a dynamically typed library. I am opposed to this library being forcibly strongly typed. That probably means we will never have a py.typed file (or whatever it is called) and that should not be a goal to work towards.

  2. The only purpose of annotations is to serve as documentation. The code dictates the annotations, not the other way around. In other words, we do not edit code to fit the annotations, we edit the annotations to fit the code. Of course, occasionally, the specific format of code may prevent defining annotations. In that case, an edit to the format (but not functionality) of the code is acceptable to allow defining annotations (the recent edits to some NamedTuple objects is one such example of this).

    Of course, type checking may occasionally reveal an otherwise unknown bug in the code. And it would only be reasonable for those bugs to be fixed. However, the fix should always be framed as a fix to the now known bug, not a change to the code to match the type annotations. In other words, I would expect a test case which fails before the fix is applied and succeeds after regardless of any type annotations.

  3. Of course, the type annotations should remain in sync with the code. I would expect some tooling to check that on the CI server. Presumably, we would add another linter like tool. The tool should never error or warn on missing annotations. The only task of the tool is to verify that the type annotations which exist agree with the code. If any fail, then a PR would not be accepted until an appropriate fix is made. Of course, an appropriate fix could mean the annotation needs to be updated, or it could mean the code needs to be updated. Is this an intentional change to the type of an object or did the type get changed inadvertently?

There are a few things we will need to do to meet the above goals.

Item 1 above requires no action expect to perhaps remove some proposed changes.

Item 2 is mostly complete. Thanks for the hard work of everyone involved.

Item 3 seems to be the problem. Originally I thought that mypy could meet our needs. After all, the page entitled Using mypy with an existing codebase in their documentation talks about how the tool can be used to progressively work through your code. However, on closer inspection, I see that the idea is to work through the code to gradually make it strongly typed. In fact, the suggestion is to do so before any annotations are added. That seems to suggest that perhaps mypy can only check annotations if the code is strongly typed. That is not a useful tool for our purpose. Unfortunately, I do not have any answers for how we can address this item. It occurs to me that it might not actually be possible to accomplish what I am proposing here.

oprypin commented 10 months ago

I'll want to sync this branch to latest if https://github.com/Python-Markdown/markdown/pull/1401 is merged, and then leave this for posterity. Not sure if I'll be able to do that if it's closed.

I did the merge, can be closed now :D