bpo-33251: Prevent ConfigParser.items returning items present in vars.

[chrBrd]

Documentation for ConfigParser.items() states:
'Items present in vars no longer appear in the result.'

This fix aligns behaviour to that specified in the documentation.

https://bugs.python.org/issue33251

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

[ambv]

If vars actually overrides existing defaults then your patch makes the original options disappear entirely. In fact, this creates a problem:

• if an existing option is shadowed by an entry in vars, should items() return the original value or the overriden value?

[ambv]

I'm leaning towards answering the question above with "overriden value". That would arguably be consistent with the docstring which states "Additional substitutions may be provided using the `vars' argument, which must be a dictionary whose contents override any pre-existing defaults."

Assuming this answer makes sense, the fix for the issue is in fact even shorter than your current patch:

• just add orig_keys = list(d.keys()) after line 848
• use those orig_keys when iterating at return time

You don't have to change vars to None, and you don't have to filter the list.

[chrBrd]

Thanks, that's a much nicer solution than mine.

Testing this has led to me discovering another discrepancy with the documentation that may be worth fixing first, however. See below.

[ambv]

This proves this never worked. The test that added ('value', 'value') dates back to 2010 (Python 3.2).

[chrBrd]

Testing @ambv 's more Pythonic solution has led me to discover another discrepancy with the documentation. I've raised a bug report.

While this issue can be fixed independently, it may be worth fixing bpo-33333 first.

[chrBrd]

This issue is resolved as a side effect of the changes made in #6567.

[ambv]

@chrBrd, Chris, I'm still up for fixing this as explained by me on the PR.

[chrBrd]

@ambv, okay Łukasz, I'll implement your recommended changes and reopen th PR. I was worried the b/c issues related to #6567 might also apply in this instance, albeit not quite to the same degree.

[chrBrd]

In case it's decided this should get merged and my CLA status still isn't showing as signed, I signed the CLA on 2018/04/10; BPO username is the same as my GitHub one.

[bedevere-bot]

@ambv: Please replace # with GH- in the commit message next time. Thanks!

[ambv]

Thanks! ✨ 🍰 ✨

Now, @chrBrd, please amend the documentation for 3.8 only to include this: https://github.com/python/cpython/pull/6494/files

(I removed it from 3.6, 3.7 and 3.8 because it incorrectly stated this happened back in Python 3.2.)

[chrBrd]

@ambv Many thanks for your patience guiding a newbie contributor Łukasz, it's been very much appreciated.

I'll get started on those doc changes (in addition to those relating to bpo-33333).

[chrBrd]

Documentation changes made in #6583.

[terryjreedy]

Chris, please sign the CLA.

Łukasz, this is more than what I and I believe most others would consider trivial and eligible for skipping the CLA. If you disagree, please discuss it on the committers list. There is a thread on the core-workflow list, Using CLA-assistant for Python, which also proposes to require CLA-signed for all merges. Keeping the current discretion requires that we agree and adhere to a fairly narrow definition of 'trivial'.

[ambv]

I agree this is no trivial change. In the comments here you can see that Chris did sign the CLA but the status didn't change timely.

@chrBrd, looks like you need to sign the CLA again, sorry!