| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I can add an entry into NEWS but I wonder if it's really useful. |
Sorry, something went wrong.
There was a problem hiding this comment.
@sblondon, am not a native English speaker I just left some nit formatting comments. Someone else can review the rest of the documentation.
Sorry, something went wrong.
Sorry, something went wrong.
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
…s and remove 3 spaces before a paragraph
There was a problem hiding this comment.
@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?
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
@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.
Sorry, something went wrong.
Co-Authored-By: Kyle Stanley <aeros167@gmail.com>
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. 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: What do you think about it? |
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
|
@aeros167 Thank you for the review ! :) (By the way, beware of burn out if you try to do too much volonteer work.) |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
@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?
Sorry, something went wrong.
|
|
||
| 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. |
There was a problem hiding this comment.
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...
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
That looks great!
Sorry, something went wrong.
There was a problem hiding this comment.
@pganssle I pushed @aeros167 's version to the repository.
Sorry, something went wrong.
There was a problem hiding this comment.
I have made the requested changes; please review again
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
|
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. (: |
Sorry, something went wrong.
|
I don't think a news entry is necessary so it's good to me. |
Sorry, something went wrong.
|
@csabella I merged a version accepted by @pganssle (see the thread #15177 (review)). @ambv never replied. |
Sorry, something went wrong.
|
Closing and re-opening to re-trigger CI checks which were spuriously failing before. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@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) |
Sorry, something went wrong.
|
Thanks everyone! |
Sorry, something went wrong.
* 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 <aeros167@gmail.com> Co-Authored-By: Paul Ganssle <p.ganssle@gmail.com>
| Back | FazBrowse Home | New Git URL |
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