bpo-37779 : Add information about the overriding behavior of ConfigParser.read #15177

sblondon · 2019-08-08T13:59:21Z

This PR adds documentation about the ability to override values when ConfigParser.read(), .read_string(), .read_dict() are called several times from the same instance of ConfigParser.

https://bugs.python.org/issue37779

…thods

sblondon · 2019-08-08T15:50:30Z

I can add an entry into NEWS but I wonder if it's really useful.
What do you think about it?

nanjekyejoannah

@sblondon, am not a native English speaker I just left some nit formatting comments. Someone else can review the rest of the documentation.

Doc/library/configparser.rst

nanjekyejoannah · 2019-08-08T19:12:05Z

cc @csabella @pganssle

Co-Authored-By: Kyle Stanley <[email protected]>

…s and remove 3 spaces before a paragraph

sblondon

@aeros167 Thanks for the rewrited paragraph, I merged it. I removed the 3 spaces before each line because the paragraph should not be indented.

The whole 'Quick Start' section use 'we', I wonder if the section should be rewritten to remove them. If it's the case, perhaps another bug report+PR is a better choice? Or reusing this one?

aeros · 2019-08-09T17:01:04Z

The whole 'Quick Start' section use 'we', I wonder if the section should be rewritten to remove them. If it's the case, perhaps another bug report+PR is a better choice?

@sblondon Yes, it should probably be in a separate PR and issue, since it''s involving a different section. Preferably, it would be done multiple PRs (single issue), instead of all at once. Smaller PRs tend to be significantly easier to review.

However, I think this issue should be marked as "newcomer friendly" (reserved mostly for first time contributors), I think it would serve as a great introduction into contributing to Python. We'll have to see what the core devs think.

aeros

@sblondon Thanks for making the changes to the phrasing.

It looks like Travis CI is failing due to the doctests starting from line 240 to 248, specifically these lines (244 and 245):

>>> topsecret.getboolean('BatchMode', fallback=True)
True

The doctests determined that the output should be False instead of True after running through the same process in the interpreter. I'm not certain if this is the correct behavior, but either way, the differing output is causing the CI to fail from the warnings.

Since this section is unrelated to this PR and in an unaltered section, I think it should be ignored for this PR and addressed in a new issue.

One more minor suggestion, the conventional format for defining parameters is to italicize them, rather than using code blocks:

Edit: Removed 3 spaces before the line part, see my comment below.

Doc/library/configparser.rst

Co-Authored-By: Kyle Stanley <[email protected]>

sblondon · 2019-08-11T09:41:19Z

@sblondon Thanks for making the changes to the phrasing.

I removed the 3 spaces before each line because the paragraph should not be indented.

The three space indentation before each line is a requirement for reST, so any .rst file must do so. See the devguide for more information.

Thanks for the link @aeros167 . However, I don't understand the sentence All reST files use an indentation of 3 spaces; no tabs are allowed. like you.
To me, it means 'if indentation is needed, use 3 spaces', not 'every line must be indented by 3 spaces'.

I think that because every .rst file I watch does not have 3 spaces indentations for the main text. Examples in cpython documentation:

Example in the devguide:

https://raw.githubusercontent.com/python/devguide/master/documenting.rst

What do you think about it?

aeros · 2019-08-11T17:50:13Z

Thanks for the link @aeros167 . However, I don't understand the sentence All reST files use an indentation of 3 spaces; no tabs are allowed. like you.
To me, it means 'if indentation is needed, use 3 spaces', not 'every line must be indented by 3 spaces'.

@sblondon Upon further inspection of the area being modified, you're correct. For some reason, I had thought that this area was surrounded by an reST directive (which causes all lines to require the 3 space indent I was referring to). It looks like there's only the initial code-block for the ini config and then doctest for the interpreter, with no directive surrounding that paragraph I suggested the changes to.

Thanks again for the PR and responsiveness to the feedback. Apologies if I caused any confusion, I'll edit my comments accordingly.

Note: I think this mistake was partially a result of me attempting to increase the volume of my PR reviews (since that's a main bottleneck for CPython). In the future, I need to make sure that I more thoroughly inspect the full section surrounding the changes, instead of looking at the diff.

sblondon · 2019-08-12T08:35:18Z

@aeros167 Thank you for the review ! :)

(By the way, beware of burn out if you try to do too much volonteer work.)

aeros · 2019-08-12T21:01:42Z

Thank you for the review ! :)
(By the way, beware of burn out if you try to do too much volonteer work.)

No problem, and yeah I definitely need to make sure that I pace myself. This is the first open source project that I've found myself incredibly motivated to help with, since Python was my first serious programming language 5+ years ago.

It can be tempting some days to just endlessly sink my free time into it, but burnout is definitely a concern. @vstinner mentioned this in a talk earlier this year at PyCon 2019.

pganssle

@ambv You are listed as a section author for this section - do you have any thoughts on the advisability of adding this section? If not, do you know who the right person would be?

Doc/library/configparser.rst

pganssle · 2019-09-11T18:46:56Z

Doc/library/configparser.rst

@@ -135,6 +135,29 @@ involves the ``DEFAULT`` section which provides default values for all other
 sections [1]_.  Note also that keys in sections are
 case-insensitive and stored in lowercase [1]_.

+If it is necessary to read several configurations, with each one having more
+priority than the previous one, an instance of :class:`ConfigParser` can be
+used to override previously defined properties and retain existing ones.


One problem I see with this is that the first few clauses of this sentence seem to be introducing a tutorial, i.e. "If you want to do X, you can achieve it with Y", but then it turns out that you are actually describing the existing behavior of ConfigParser rather than providing a recipe or something.

I would recommend rewriting this as an introduction to the properties of a ConfigParser, like so:

It is also possible to read several configurations into a single :class:`ConfigParser` object, in which case the most recent configuration has priority over the previous ones; conflicting keys will be taken from the new configuration and existing keys will be retained.

That's mostly an example, I'm not super happy with that particular wording...

@pganssle:

That's mostly an example, I'm not super happy with that particular wording...

What do you think of this wording?

It is possible to read several configurations into a single :class:ConfigParser, where the most recently added configuration has the highest priority. Any conflicting keys are taken from the more recent configuration while the previously existing keys are retained.

That looks great!

@pganssle I pushed @aeros167 's version to the repository.

I have made the requested changes; please review again

bedevere-bot · 2019-09-11T18:47:58Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-Authored-By: Paul Ganssle <[email protected]>

aeros · 2019-09-12T20:13:48Z

Added a skip news label since this is a documentation change, but you can optionally add one if you'd like to @sblondon. Thanks again for the PR. (:

sblondon · 2019-09-13T17:53:31Z

I don't think a news entry is necessary so it's good to me.

sblondon · 2020-08-22T13:17:51Z

@csabella I merged a version accepted by @pganssle (see the thread #15177 (review)). @ambv never replied.
What can I do to help the merge occurs?

ambv · 2020-09-16T11:16:05Z

Closing and re-opening to re-trigger CI checks which were spuriously failing before.

ambv · 2020-09-16T11:29:04Z

Looks like the failure is real: the documentation doctest fails with:

File "library/configparser.rst", line 246, in default
Failed example:
    topsecret.getboolean('BatchMode', fallback=True)
Expected:
    False
Got:
    True

...

 Makefile:127: recipe for target 'doctest' failed
make: *** [doctest] Error 1
make: Leaving directory '/home/runner/work/cpython/cpython/Doc'

@sblondon, please confirm you can successfully build the documentation locally by running this in the main directory of your cpython checkout:

make -C Doc/ PYTHON=../python doctest suspicious html

sblondon · 2020-09-18T14:53:53Z

@ambv I renamed the config object created by the patch so now the first instance is not overwritten and the failing test uses it like when the patch wasn't there. Is this workaround enough?

I checked the fix on my computer with:

>>> import doctest
>>> doctest.testfile("Doc/library/configparser.rst")
TestResults(failed=0, attempted=88)

sblondon · 2020-09-24T08:29:26Z

Thanks everyone!

* origin/master: (27 commits) bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388) bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387) bpo-41833: threading.Thread now uses the target name (pythonGH-22357) bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633) bpo-33822: Update IDLE section of What's New 3.8 (pythonGH-22383) bpo-41844: Add IDLE section to What's New 3.9 (GN-22382) bpo-41841: Prepare IDLE News for 3.10 (pythonGH-22379) bpo-37779 : Add information about the overriding behavior of ConfigParser.read (pythonGH-15177) bpo-40170: Use inline _PyType_HasFeature() function (pythonGH-22375) bpo-40941: Fix stackdepth compiler warnings (pythonGH-22377) bpo-40941: Fix fold_tuple_on_constants() compiler warnings (pythonGH-22378) bpo-40521: Fix PyUnicode_InternInPlace() (pythonGH-22376) bpo-41834: Remove _Py_CheckRecursionLimit variable (pythonGH-22359) bpo-1635741, unicodedata: add ucd_type parameter to UCD_Check() macro (pythonGH-22328) bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (pythonGH-22220) bpo-41513: Improve order of adding fractional values. Improve variable names. (pythonGH-22368) bpo-41816: `StrEnum.__str__` is `str.__str__` (pythonGH-22362) bpo-35764: Rewrite the IDLE Calltips doc section (pythonGH-22363) bpo-41810: Reintroduce `types.EllipsisType`, `.NoneType` & `.NotImplementedType` (pythonGH-22336) bpo-41602: raise SIGINT exit code on KeyboardInterrupt from pymain_run_module (python#21956) ...

…rser.read (pythonGH-15177) Co-Authored-By: Kyle Stanley <[email protected]> Co-Authored-By: Paul Ganssle <[email protected]>

Add information about the overriding behavior of ConfigParser.read me…

3ff9d28

…thods

the-knights-who-say-ni added the CLA signed label Aug 8, 2019

bedevere-bot added docs Documentation in the Doc dir awaiting review labels Aug 8, 2019

nanjekyejoannah reviewed Aug 8, 2019

View reviewed changes

Doc/library/configparser.rst Show resolved Hide resolved

Doc/library/configparser.rst Show resolved Hide resolved

Doc/library/configparser.rst Outdated Show resolved Hide resolved

Doc/library/configparser.rst Outdated Show resolved Hide resolved

sblondon and others added 2 commits August 9, 2019 11:15

Update Doc/library/configparser.rst

b3ea855

Co-Authored-By: Kyle Stanley <[email protected]>

aesthetic: add another line before a paragraph, remove trailing space…

73ea042

…s and remove 3 spaces before a paragraph

sblondon commented Aug 9, 2019

View reviewed changes

Fix formatting errors (line length and empty lines)

13ad7ab

aeros suggested changes Aug 9, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 9, 2019

aeros reviewed Aug 9, 2019

View reviewed changes

Doc/library/configparser.rst Outdated Show resolved Hide resolved

Parameter function are stylized in italic

3774983

Co-Authored-By: Kyle Stanley <[email protected]>

pganssle requested changes Sep 11, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 11, 2019

sblondon and others added 2 commits September 12, 2019 10:54

Update Doc/library/configparser.rst

8d9a084

Co-Authored-By: Paul Ganssle <[email protected]>

Wording improvement

42dc02a

aeros added the skip news label Sep 12, 2019

aeros approved these changes Sep 13, 2019

View reviewed changes

csabella requested review from pganssle and ambv January 12, 2020 13:44

ambv closed this Sep 16, 2020

ambv reopened this Sep 16, 2020

rename a variable to fix a doctest

196d4f8

ambv merged commit 48b0b1b into python:master Sep 23, 2020

bedevere-bot removed the awaiting changes label Sep 23, 2020

sblondon deleted the doc-configparser-several-read-calls branch September 24, 2020 08:29

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020

bpo-37779 : Add information about the overriding behavior of ConfigPa…

d5b7fc6

…rser.read (pythonGH-15177) Co-Authored-By: Kyle Stanley <[email protected]> Co-Authored-By: Paul Ganssle <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37779 : Add information about the overriding behavior of ConfigParser.read #15177

bpo-37779 : Add information about the overriding behavior of ConfigParser.read #15177

sblondon commented Aug 8, 2019 •

edited by bedevere-bot

Loading

sblondon commented Aug 8, 2019

nanjekyejoannah left a comment

nanjekyejoannah commented Aug 8, 2019

sblondon left a comment

aeros commented Aug 9, 2019 •

edited

Loading

aeros left a comment •

edited

Loading

sblondon commented Aug 11, 2019

aeros commented Aug 11, 2019 •

edited

Loading

sblondon commented Aug 12, 2019

aeros commented Aug 12, 2019 •

edited

Loading

pganssle left a comment

pganssle Sep 11, 2019

aeros Sep 11, 2019 •

edited

Loading

pganssle Sep 12, 2019

sblondon Sep 12, 2019

sblondon Sep 12, 2019

bedevere-bot commented Sep 11, 2019

aeros commented Sep 12, 2019

sblondon commented Sep 13, 2019

sblondon commented Aug 22, 2020

ambv commented Sep 16, 2020

ambv commented Sep 16, 2020

sblondon commented Sep 18, 2020

sblondon commented Sep 24, 2020

bpo-37779 : Add information about the overriding behavior of ConfigParser.read #15177

bpo-37779 : Add information about the overriding behavior of ConfigParser.read #15177

Conversation

sblondon commented Aug 8, 2019 • edited by bedevere-bot Loading

sblondon commented Aug 8, 2019

nanjekyejoannah left a comment

Choose a reason for hiding this comment

nanjekyejoannah commented Aug 8, 2019

sblondon left a comment

Choose a reason for hiding this comment

aeros commented Aug 9, 2019 • edited Loading

aeros left a comment • edited Loading

Choose a reason for hiding this comment

sblondon commented Aug 11, 2019

aeros commented Aug 11, 2019 • edited Loading

sblondon commented Aug 12, 2019

aeros commented Aug 12, 2019 • edited Loading

pganssle left a comment

Choose a reason for hiding this comment

pganssle Sep 11, 2019

Choose a reason for hiding this comment

aeros Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

pganssle Sep 12, 2019

Choose a reason for hiding this comment

sblondon Sep 12, 2019

Choose a reason for hiding this comment

sblondon Sep 12, 2019

Choose a reason for hiding this comment

bedevere-bot commented Sep 11, 2019

aeros commented Sep 12, 2019

sblondon commented Sep 13, 2019

sblondon commented Aug 22, 2020

ambv commented Sep 16, 2020

ambv commented Sep 16, 2020

sblondon commented Sep 18, 2020

sblondon commented Sep 24, 2020

sblondon commented Aug 8, 2019 •

edited by bedevere-bot

Loading

aeros commented Aug 9, 2019 •

edited

Loading

aeros left a comment •

edited

Loading

aeros commented Aug 11, 2019 •

edited

Loading

aeros commented Aug 12, 2019 •

edited

Loading

aeros Sep 11, 2019 •

edited

Loading