Skip to content

Make configparser.RawConfigParser use SectionProxy where appropriate #1527

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

Merged
merged 6 commits into from
Aug 21, 2017

Conversation

OddBloke
Copy link
Contributor

@OddBloke OddBloke commented Aug 6, 2017

This reflects the code in the cpython tree and makes the following
(valid) code type-check correctly:

from configparser import ConfigParser

config = ConfigParser()
config.read_dict({'section': {'key': 'false'}})
assert config['section'].getboolean('key') is False

OddBloke added a commit to OddBloke/jenkins-job-linter that referenced this pull request Aug 6, 2017
@OddBloke OddBloke changed the title Make configparser.RawConfigParser.__getitem__ return a SectionProxy Make configparser.RawConfigParser use SectionProxy where appropriate Aug 7, 2017
@OddBloke
Copy link
Contributor Author

OddBloke commented Aug 7, 2017

Also relevant to getting the types in check: python/cpython#3012

def items(self, *, raw: bool = ..., vars: _section = ...) -> AbstractSet[Tuple[str, SectionProxy]]: ...

@overload
def items(self, section: str, raw: bool = ..., vars: _section = ...) -> Iterable[Tuple[str, str]]: ...
Copy link
Member

Choose a reason for hiding this comment

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

It actually returns a list. Any reason not to use List in the annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason I can think of; updated.

@reddraggone9
Copy link

I created a file test.py with the code in the OP and added the following line to the end:

reveal_type(config['section'].getboolean('key'))

Using this branch of typeshed, mypy gives me

test.py:6: error: Revealed type is 'Any'

If I understand this correctly, using the current definition for SectionProxy causes getboolean() to fall back on __getattr__(), which is a very loose definition since SectionProxy pulls its get methods from the parser that created it so that parser subclasses have a nicer time. I tried out mypy for the first time today and ran into this while trying to get --strict to pass on the file I was messing around with, so sorry if I'm missing something.

Bottom line, it seems unfortunate that mypy doesn't know that getboolean() returns a bool. Can we fix that?

@OddBloke
Copy link
Contributor Author

@reddraggone9 That is something that needs fixing, but AFAICT this PR doesn't regress that behaviour.

Indeed, without this PR, I get the following output from mypy:

test.py:5: error: Mapping[str, str] has no attribute "getboolean"
test.py:6: error: Revealed type is 'Any'
test.py:6: error: Mapping[str, str] has no attribute "getboolean"

Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

Rebase and I'll merge.

# This is incompatible with Mapping so we ignore the type.
def items(self, section: str = ..., raw: bool = ..., vars: _section = ...) -> Iterable[Tuple[str, _section]]: ... # type: ignore
@overload
def items(self, *, raw: bool = ..., vars: _section = ...) -> AbstractSet[Tuple[str, SectionProxy]]: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, return types that are as concrete as possible, and accept types in arguments that are as generic as possible.

Daniel Watkins added 6 commits August 21, 2017 17:12
This reflects the code in the cpython tree and makes the following
(valid) code type-check correctly:

```
from configparser import ConfigParser

config = ConfigParser()
config.read_dict({'section': {'key': 'false'}})
assert config['section'].getboolean('key') is False
```
Because .items() uses __getitem__ to produce second item in each tuple.
TL;DR, we're hitting python/mypy#3805 when
implemented correctly as an override, and
python/mypy#1855 when we try to work around
that with a union.
@OddBloke
Copy link
Contributor Author

@ambv Done.

@ambv ambv merged commit 24a7bfb into python:master Aug 21, 2017
OddBloke added a commit to OddBloke/jenkins-job-linter that referenced this pull request Aug 29, 2017
python/typeshed#1527 has landed in mypy master,
which is what we use in tests.
OddBloke added a commit to OddBloke/jenkins-job-linter that referenced this pull request Aug 29, 2017
python/typeshed#1527 has landed in mypy master,
which is what we use in tests.
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.

4 participants