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

Update signature of ConfigParser.get() #501

Merged
merged 14 commits into from
Aug 29, 2016
Merged

Update signature of ConfigParser.get() #501

merged 14 commits into from
Aug 29, 2016

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Aug 26, 2016

Issue #492

This signature seems to break the protocol of MutableMappting[str, _section]

@gvanrossum
Copy link
Member

Thanks for trying to tackle this one!

I've thought about this more, and I think the right thing to do (for now) is to add # type: ignore to the definition of get(). (The overload games won't work.) This will shut up mypy's complaint about get not being incompatible with MutableMapping (#492) but calls of it will still correctly be type-checked with the (str, str) -> str signature (IOW the # type: ignore does not make the definition of get equal to (*Any) -> Any).

The problem here is quite deep, you can read more about it at python/mypy#1237 and python/typing#241. But as a short-term solution, the # type: ignore should work just fine.

Of course if you pass a ConfigParser object to a function that requires a Mapping and that function happens to call get, it's a problem, and the type checker doesn't warn you. But no matter what solution we choose that will be a problem, unless we decide to not inherit from [Mutable]Mapping after all -- but that goes against the public documentation. (If we end up deciding that the Mapping inheritance is only "implementation inheritance" then the right thing to do in the stub would indeed be to not declare MutableMapping as a base class, but that might have other unpleasant consequences. Life's rarely fully consistent! :-)

I think there are other problems with configparser.pyi too (e.g. read() takes a string too, and RawConfigParser is not there at all) but those can wait until another PR.

@elazarg
Copy link
Contributor Author

elazarg commented Aug 29, 2016

I can't find the documentation about type: ignore on function definition.

@gvanrossum
Copy link
Member

It should look like this:

def get(blah blah) -> blah: # type: ignore
    ...

@gvanrossum
Copy link
Member

You should still specify the correct signature (according to configparser docs, ignoring MutableMapping).

# should be def get(self, section: str, option: str, *, raw: bool = ..., vars: _section = ..., fallback: str = ...) -> str: ...
# but it is incompatible with MutableMapping
def get(self, *args, **kwargs): # type: ignore
# type: ignore since this is incompatible with MutableMapping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't start this comment with exactly # type: ignore since that may trigger other tools looking for # type: ignore on a new line. Just prefix with "Uses" or something.

ConfigParser <: RawConfigParser, with __init__ changes:
* delimiters and onward are kwargs only
* no converters at the end
@gvanrossum gvanrossum merged commit 59585bb into python:master Aug 29, 2016
@gvanrossum
Copy link
Member

Thanks! Phew!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants