pfmoore / editables

MIT License
39 stars 11 forks source link

Add type annotations #14

Closed pfmoore closed 1 year ago

pfmoore commented 2 years ago

pyright is complaining about redirector.py, although mypy is fine with it. This appears to be a bug in typeshed: https://github.com/python/typeshed/issues/2468

layday commented 2 years ago

If you want consumers of this library to be able to use type annotations, stubs will do the trick, but mypy will not validate your types against the source code. You could change up all the parameter names in the stub file and mypy would not report any errors. There is another (currently undocumented) script that comes with mypy that'll do that for you, stubtest, though I've never used it personally.

Remember that you need to add the pyi files to the distribution, setuptools won't include them automatically.

pyright is complaining about redirector.py, although mypy is fine with it. This appears to be a bug in typeshed: python/typeshed#2468

The redirector is not part of the public API, is it? You could leave it unannotated.

layday commented 2 years ago

Output of stubtest editables:

error: editables.EditableProject.files is not present in stub
Stub:
MISSING
Runtime: at line 41 in file /Users/dimitris/code/python-packaging/editables/src/editables/__init__.py
<function EditableProject.files at 0x10691f0d0>

You will need to create src/editables/py.typed before it's able to detect the stub files.

pfmoore commented 2 years ago

If you want consumers of this library to be able to use type annotations, stubs will do the trick, but mypy will not validate your types against the source code.

To be honest, I don't personally care about whether consumers can use type annotations. I'm willing to add them if users want them, but I'm not bothered unless there's a demand. I'm assuming that you want to use annotations, and the lack is a problem for you, but if not, I don't want to waste your time - I'm happy to wait until someone with a need for this speaks up.

I'm pretty disappointed that mypy won't validate types unless I add them into the code. I'm definitely against doing that because the backward compatibility situation with types is so bad. (See previous comments about PathLike...) In essence, that means that this project gets no ongoing benefit itself from adding stubs, which is a shame (particularly as pyright did pick up one minor issue).

Remember that you need to add the pyi files to the distribution, setuptools won't include them automatically.

Do you have an example of how to do that? Preferably with some sort of tests to validate that the stubs get added correctly (I'd intended to add a mypy run to the test suite, but if that won't use stub files, it doesn't actually help). I nearly always mess up when I try to get setuptools to do anything out of the ordinary.

You will need to create src/editables/py.typed before it's able to detect the stub files.

Thanks, fixed

I'm happy to continue trying to get this to work, but as it doesn't benefit the project in terms of better testing, or early detection of issues, I'm not particularly worried if it has to wait a while. And I do want it to have minimal maintenance impact, hence my preference for stub files and some form of tests that will ensure the stubs don't get out of sync with the code.

layday commented 2 years ago

I'm assuming that you want to use annotations, and the lack is a problem for you

I was using it in the frontends editables PoC back when but I'm not using it now. I assume that backends which depend on editables for PEP 660 support are either not using mypy or have marked editables as untyped in mypy so that it has type Any.

pfmoore commented 2 years ago

OK. Thanks for your help with this, let's leave it until someone with a current use case can help work through the issues.

Also:

The redirector is not part of the public API, is it? You could leave it unannotated.

Probably, I guess...? This is a good example of why I want someone who's actually using type checkers and having an issue with this to comment. The redirector is an internal detail, yes, but it is public in the sense that the bootstrap code imports it (that's why there's a runtime dependency on editables if you don't stick to the APIs that are covered by the .pth file mechanism). I don't know whether someone using this and testing their code with a type checker would need annotations for the redirector.

As a stopgap, though, removing redirector.pyi might be a reasonable thing to do.

If we were able to use type checking to catch errors in the library code itself, I'd definitely want the redirector to be type checked, as that's one of the tricky bits of the code - ironically, I'd want the type checker to assure that I'm conforming to the importlib protocols, which is why the typeshed bug is so frustrating to me...

layday commented 2 years ago

FWIW, when I use annotations in my personal projects, I only ever annotate function parameters (see e.g. https://github.com/layday/github-wow-addon-catalogue/blob/main/collect.py). Everything else the type checker can infer, so that's really all you need to reap the benefits of static typing. This is especially true if you are using Pyright (which does, unfortunately, depend on NodeJS); mypy's "strict" mode requires that you provide return type annotations.

pfmoore commented 1 year ago

Messed this up. Let's try again in a new PR.