Skip to content

Add missing None return to RawConfigParser.get #8380

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 10 commits into from
Jul 26, 2022
Merged

Add missing None return to RawConfigParser.get #8380

merged 10 commits into from
Jul 26, 2022

Conversation

kkirsche
Copy link
Contributor

Fix #7476

This adds support to RawConfigParser.get to return None as seen on these lines in cPython:
https://github.com/python/cpython/blob/main/Lib/configparser.py#L773-L774

Fix #7476

This adds support to RawConfigParser.get to return None as seen on these lines in cPython:
https://github.com/python/cpython/blob/main/Lib/configparser.py#L773-L774
@kkirsche kkirsche changed the title Add missing Nonereturn to RawConfigParser.get Add missing None return to RawConfigParser.get Jul 24, 2022
pre-commit-ci bot and others added 2 commits July 24, 2022 10:56
Update `SectionProxy.get` with the missing `| None` from `RawConfigParser`.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Jul 24, 2022

Presumably ConfigParser shouldn't inherit this behaviour. Usually you just want to parse a config file with ConfigParser, and in that case getting None seems to be impossible:

>>> p = configparser.ConfigParser()
>>> p.read_string("[foo]\nempty =")
>>> p.get("foo", "empty")
''
>>> p.get("foo", "asdasd")
Traceback (most recent call last):
   ...
configparser.NoOptionError: No option 'asdasd' in section: 'foo'

A couple ways to fix this:

  • Add another get method to ConfigParser that doesn't have | None.
  • Return str | Any, meaning that the user has to be prepared to get a string, but they can handle other types too if they want.

You will also have to adjust SectionProxy. Maybe make it generic, so that it can be a MutableMapping[str, str] for ConfigParser but MutableMapping[str, str | None] for RawConfigParser?

@github-actions

This comment has been minimized.

@kkirsche
Copy link
Contributor Author

I've added the overloads for ConfigParser. Regarding SectionProxy, if I make it a generic that can accept either, SectionProxy no longer is compatible with `Mapping[str, str]

Regarding making section proxy generic, I can do that but it seems to really ripple throughout as we have to adjust the __getitem__ implementations, and related TypeAliases like _Parser to handle how it can change.

@Akuli
Copy link
Collaborator

Akuli commented Jul 25, 2022

Indeed, the inheritance is a bit unfortunate: RawConfigParser should behave like MutableMapping[str, SectionProxy[str | None]] but its subclass ConfigParser should behave like MutableMapping[str, SectionProxy[str]]. I don't think there's any way to express this. Maybe we should just return str | Any from SectionProxy's .get(), which makes it usable for both ConfigParser and RawConfigParser, although you won't get errors if you use RawConfigParser and you forget to handle None.

@github-actions

This comment has been minimized.

@overload
def get(self, section: str, option: str, *, raw: bool = ..., vars: _Section | None = ..., fallback: _T) -> str | _T: ...
def get(
self, section: str, option: str, *, raw: bool = ..., vars: _Section | None = ..., fallback: _T
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at zproject/config.py in zulip (from last mypy_primer output), mypy seems to think that _T is str, and complains when you do fallback=None. I will try to fix that.

Suggested change
self, section: str, option: str, *, raw: bool = ..., vars: _Section | None = ..., fallback: _T
self, section: str, option: str, *, raw: bool = ..., vars: _Section | None = ..., fallback: _T | None

Copy link
Collaborator

Choose a reason for hiding this comment

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

I reverted this. Returning a possibly None value is good if you pass fallback=None.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@Akuli
Copy link
Collaborator

Akuli commented Jul 26, 2022

This still seems a bit too disruptive to me: all mypy_primer results above (you need to click "1 similar comment") are usages of RawConfigParser objects that probably don't contain any Nones, because to get a None value in RawConfigParser, you would have to manually insert it instead of just reading a file.

We have a couple options to choose from:

  • Just tell everyone to stop using RawConfigParser. This isn't great, because it's not necessarily the fault of whoever gets mypy errors. For example, flake8 provides a RawConfigParser object for plugin authors.
  • Use | Any in RawConfigParser too

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Akuli
Copy link
Collaborator

Akuli commented Jul 26, 2022

Thanks!

@Akuli Akuli merged commit 41435ef into python:master Jul 26, 2022
@kkirsche kkirsche deleted the patch-4 branch July 27, 2022 11:19
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.

RawConfigParser.get should return str | None, not str
2 participants