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

How much do we care about positional-only args? #3693

Closed
hauntsaninja opened this issue Jan 30, 2020 · 3 comments
Closed

How much do we care about positional-only args? #3693

hauntsaninja opened this issue Jan 30, 2020 · 3 comments
Labels
project: policy Organization of the typeshed project

Comments

@hauntsaninja
Copy link
Collaborator

It's fairly common for arguments that were positional-only to become no longer positional-only between minor versions of Python. For example, zipimport between 3.7 and 3.8:

~/dev/mypy/scripts stubtest2 λ python3.8 stubtest.py --custom-typeshed-dir ~/dev/typeshed --concise zipimport
zipimport.zipimporter.__init__ is inconsistent, stub argument "archivepath" differs from runtime argument "path"
zipimport.zipimporter.find_loader is not present in stub
zipimport.zipimporter.find_module is inconsistent, runtime argument "path" has a default value of type None, which is incompatible with stub argument type builtins.str
zipimport.zipimporter.get_resource_reader is not present in stub

~/dev/mypy/scripts stubtest2 λ python3.7 stubtest.py --custom-typeshed-dir ~/dev/typeshed --concise zipimport
zipimport.zipimporter.find_loader is not present in stub
zipimport.zipimporter.find_module is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")
zipimport.zipimporter.find_module is inconsistent, runtime argument "path" has a default value of type None, which is incompatible with stub argument type builtins.str
zipimport.zipimporter.find_module is inconsistent, stub argument "path" should be positional-only (rename with a leading double underscore, i.e. "__path")
zipimport.zipimporter.get_code is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")
zipimport.zipimporter.get_data is inconsistent, stub argument "pathname" should be positional-only (rename with a leading double underscore, i.e. "__pathname")
zipimport.zipimporter.get_filename is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")
zipimport.zipimporter.get_resource_reader is not present in stub
zipimport.zipimporter.get_source is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")
zipimport.zipimporter.is_package is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")
zipimport.zipimporter.load_module is inconsistent, stub argument "fullname" should be positional-only (rename with a leading double underscore, i.e. "__fullname")

We can branch on sys.version_info to capture this — and I'm happy to do that — but figured I'd check whether this is actually desired or not before doing something that's pretty nitpicky. The upside here is accuracy, the downside here is it makes the stubs less readable and it can mask more important differences between Pythons.

An alternative would be to only aim for positional-only arg accuracy in the latest Python release (with anything else being discretionary). One way this would manifest is if we introduced stubtest to typeshed CI, we could run against older versions of Python with --ignore-positional-only.

While I'm here, some other questions:

  • Is there a plan for using PEP 570 syntax in typeshed (does that need a change in typed_ast)?
  • It seems we should mark several methods in typing.pyi as using positional-only args, e.g, Mapping.get?
  • I've currently been doing a lot of module by module PRs, where these include positional-only args changes if necessary as a commit within the PR. Does this work well for reviewing / are there things I could do to make life easier (in general, not just wrt positional-only args 🙂 )?
@JelleZijlstra
Copy link
Member

I would prefer to just go with the latest version in cases like this, since there is fairly little risk of actual code attempting to use these arguments as keyword arguments, and as you say it would complicate the stubs. I'm interested to hear other opinions though.

As for your other questions:

  • We currently cannot use PEP 570 syntax in typeshed because typed-ast does not support it. typed-ast doesn't need to support it for mypy, because as of 3.8 CPython itself exposes a typed ast. @srittau has tried to add PEP 570 support to typed-ast in the past but couldn't get it to work.
  • Yes, various protocol methods probably should be marked as positional-only (this just came up on the mypy issue tracker too: Sequence type does neither match the ABC nor the implementation of tuple mypy#8345).
  • I like module-by-module PRs; bigger ones would be hard to review.

@srittau
Copy link
Collaborator

srittau commented Jan 31, 2020

As an addendum to what Jelle wrote: I wouldn't want to reduce accuracy for positional-only arguments, i.e. if something already has branches, I wouldn't want to accept a patch that removes those. But PR that changes the args from one kind to the other, because it has changed in later Python versions, is fine.

@hauntsaninja
Copy link
Collaborator Author

Sounds good, thanks both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

No branches or pull requests

3 participants