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

Enable checking against mypy with minimal set of fixes #1402

Closed oprypin closed 10 months ago

oprypin commented 10 months ago

4 significant checks are disabled:

waylan commented 10 months ago

I'm sorry, this is is still way to much rewriting of code. I will not under any circumstances change the way I write code in a dynamically typed language to suite a type checker. The checker should only be verifying that existing annotations are correct.

The only edits which are acceptable are edits to the annotations. If you can't make that work, then mypy is not the right tool.

I see three ways forward:

  1. Configure mypy to do what I want (seems unlikely).
  2. Find a different tool which does what I want.
  3. Do nothing.
waylan commented 10 months ago

So, it seems that if you can configure mypy to not require any of the code changes included here, then it would be something we can use.

However, I will note that simply commenting lines of code may not be the best solution in some cases. For example. there is little difference in def __init__(self) -> None: and def __init__(self): # ignore. I would expect to be able to do def __init__(self):. And the same goes for every time we use Python's dynamic nature. There shouldn't need to be a comment on every such line. A global config option would be preferable. If mypy can't do that, then its not the right tool for the job.

facelessuser commented 10 months ago

Yes, it seems that the projects goals and this PR's goals are a bit at odds. It sounds like Python Markdown only cares about checking that types are defined, but type checking itself is not desired.

I will add one point though. There seems to be some disdain for explicitly defining a function has no return, but if that is disabled, it seems you have no actual way of checking if a return is required and missed? Maybe I'm wrong about this?

waylan commented 10 months ago

I will add one point though. There seems to be some disdain for explicitly defining a function has no return, but if that is disabled, it seems you have no actual way of checking if a return is required and missed? Maybe I'm wrong about this?

I'm okay with that. The only thing that should be checked is that existing annotations are correct. Anything that is not annotated should be ignored. Seems like a simple concept. I don't understand why that is so hard... except that perhaps it is because the goal of the type checker is to enforce static typing, which is something I have repeatedly said I am not interested in.

oprypin commented 10 months ago

The only thing that should be checked is that existing annotations are correct.

In which way, just purely conceptually, do you think this is supposed to be accomplished? What is "correct" in isolation?

The only known way to do this is to check code that uses annotated members. If something doesn't match up then it shows an error, and you need to check if it's the code that is wrong or the annotation that is wrong.

facelessuser commented 10 months ago

I'm okay with that. The only thing that should be checked is that existing annotations are correct. Anything that is not annotated should be ignored. Seems like a simple concept. I don't understand why that is so hard... except that perhaps it is because the goal of the type checker is to enforce static typing, which is something I have repeatedly said I am not interested in.

That's fair, and it is possible I've misinterpreted some interaction. So yeah, I'm good with that.

waylan commented 10 months ago

The only known way to do this is to check code that uses annotated members. If something doesn't match up then it shows an error, and you need to check if it's the code that is wrong or the annotation that is wrong.

Yes, when its annotated that is reasonable. And when it is not annotated, it should be ignored.

In other words, suppose we have to functions, one of which is annotated and one which is not. The tool would see a line of code which calls the annotated function, It would see the annotation and confirm that the return value matches up. However, when it sees the unannotated function is would not confirm anything about the return value as there is nothing to confirm. That is what I expect to happen.

In the first case, the assumption would then be that perhaps the annotation is wrong. However, we could check for a potential bug in the code and fix that instead if need be. In the second case, we are alerted to nothing as it has been for many years.

And no, I don't want to have to add an ignore comment to every one of those lines. The tool should just ignore them either by default or via a global setting.

oprypin commented 10 months ago

I think mypy in the current config matches what you describe - it validates only things that are annotated.

Which of the comment threads don't match this? There was an example where something clearly annotated as str | Element is being passed to something clearly annotated as str. How is that not a correct finding that either the annotation is wrong or the code can have a problem?

Although a big snag might be in the fact that the whole standard library is annotated, so usages (inside annotated functions) are checked against it as well.

And there was one consistently mentioned limitation of mypy - that it can't cope with reassigning a different-typed value to the same variable name, and it should be able to. I have replied with "limitation" to those threads. But it's a conceptually different problem. And if we do look at it separately, perhaps this pull request can be said to have only this as a blocker.

waylan commented 10 months ago

Which of the comment threads don't match this? There was an example where something clearly annotated as str | Element is being passed to something clearly annotated as str. How is that not a correct finding that either the annotation is wrong or the code can have a problem?

I've never suggested that that was the case. In fact, if the issue is that the annotation was wrong, I have made no objection. However, I have consistently stated that if the code is wrong, then each individual instance should be addressed as a separate bug, not included wholesale in this large PR. This PR (or one like it) should not be changing code. only annotations. And yet, you have repeatedly ignored that request.

And there was one consistently mentioned limitation of mypy - that it can't cope with reassigning a different-typed value to the same variable name, and it should be able to. I have replied with "limitation" to those threads. But it's a conceptually different problem. And if we do look at it separately, perhaps this pull request can be said to have only this as a blocker.

And a very big blocker is it, I have stated repeatedly that I will not change this lib to be statically typed, which mypy requires. In other words, I will not change the code to appease a tool which is designed to enforce something we are not enforcing. And it is unreasonable to expect us to ignore-comment every single dynamically typed object in our code line-by-line. If mypy had a global option to not enforce that, then it wouldn't be a tool which enforced static types. Therefore, it seems that if does not fit our needs.