FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-38741: Definition of multiple ']' in header configparser by jsnklln · Pull Request #17129 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-38741: Definition of multiple ']' in header configparser - #17129

Merged
ambv merged 4 commits into
python:mainfrom
jsnklln:configparser_handle_brace_in_header
Jul 13, 2021
ConversationCommits (4)ChecksFiles changed
Merged

bpo-38741: Definition of multiple ']' in header configparser#17129
ambv merged 4 commits into
python:mainfrom
jsnklln:configparser_handle_brace_in_header

Conversation

jsnklln commented Nov 12, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

I've changed up the regex for finding section headers.
I don't know if this covers every case but it covers
all the cases I could come up with. The docs don't really
call out if ] should be allowed in section headers but
I can't think of any reason it shouldn't be.

This change keeps the existing failure if you try and
define a section with no name, like [].

https://bugs.python.org/issue38741

bpo-38741: Definition of multiple ']' in header configparser
df7784b 
I've changed up the regex for finding section headers.
I don't know if this covers every case but it covers
all the cases I could come up with.  The docs don't really
call out if ] should be allowed in section headers but
I can't think of any reason it shouldn't be.

This change keeps the existing failure if you try and
define a section with no name, like [].
brandtbucher added the type-bug An unexpected behavior, bug, or error label Nov 12, 2019

brandtbucher commented Nov 12, 2019

Copy link
Copy Markdown
Member

Thanks @jsnklln!

brandtbucher requested changes Nov 12, 2019
View reviewed changes

brandtbucher left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The patch itself looks good, but I don't know enough about this module to decide if this behavior is technically correct or not. @ambv, thoughts?

Also, this probably needs a test or two. Can you include them in test_configparser.py?

jsnklln commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

@brandtbucher adding some tests sounds like fun. I'm totally open to opinions on how to best handle it.

mark99i commented Nov 13, 2019

Copy link
Copy Markdown

its is a my commit!
46751c6
or was it not customary to point to the source?

jsnklln commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

@mark99i I didn't realize we were working on the same bug. There's not a PR on the bug tracker and I'm pretty sure I pulled from changes master yesterday but I hay not have.

One note. Using .* allows [] to be considered valid where with the previous version it threw an exception. ConfigParser doesn't allow definitions without a section, I'm pretty sure that's still true, so I don't know where those would go if they were allowed.

Jason Killen added 2 commits November 13, 2019 11:19
Added tests to ensure sections with ] in them are
fa270a6 
read correctly.  Included a check for key=value because
the original issue mentioned that it might not be working.
Merge branch 'configparser_handle_brace_in_header' of https://github.…
aa89701 
…com/jsnklln/cpython into configparser_handle_brace_in_header

jsnklln commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

@brandtbucher tests have been pushed.

brandtbucher approved these changes Nov 13, 2019
View reviewed changes

brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @jsnklln! I recommended that #17086 be closed in favor of this, since it contains tests and handles the empty case.

jsnklln commented Nov 13, 2019

Copy link
Copy Markdown
Contributor Author

@brandtbucher thanks. @mark99i sorry we got our wires crossed up, I'll gladly split the credit.

jsnklln commented Dec 4, 2019

Copy link
Copy Markdown
Contributor Author

@brandtbucher is this waiting on something to be merged? Not being pushy just want to make sure I've got all my t's dotted.

brandtbucher requested a review from ambv February 21, 2020 04:21

brandtbucher commented Jul 17, 2020

Copy link
Copy Markdown
Member

Closing and reopening to trigger CI.

brandtbucher reopened this Jul 17, 2020

brandtbucher commented Jul 17, 2020

Copy link
Copy Markdown
Member

@ambv This patch is simple and looks fine to me. Do you mind clearing up whether or not it is correct to allow ] in header names? If so, it can probably be merged.

ambv merged commit 2924bb1 into python:main Jul 13, 2021

miss-islington commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

Thanks @jsnklln for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot commented Jul 13, 2021

Copy link
Copy Markdown

GH-27110 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 13, 2021
bpo-38741: Definition of multiple ']' in header configparser (pythonG…
7e2f31c 
…H-17129)

Co-authored-by: Jason Killen <jason.killen@windsorcircle.com>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
(cherry picked from commit 2924bb1)

Co-authored-by: jsnklln <jsnklln@gmail.com>

ambv commented Jul 13, 2021

Copy link
Copy Markdown
Contributor

Sorry for taking so long to look at this 🤦🏻‍♂️

ambv pushed a commit that referenced this pull request Jul 13, 2021
bpo-38741: Definition of multiple ']' in header configparser (GH-17129)…
1cc6769 
… (#27110)

Co-authored-by: Jason Killen <jason.killen@windsorcircle.com>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
(cherry picked from commit 2924bb1)

Co-authored-by: jsnklln <jsnklln@gmail.com>
pablogsal mentioned this pull request Jul 18, 2021
Closed
abravalheri mentioned this pull request Oct 5, 2023
Closed
Delgan mentioned this pull request Oct 7, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

brandtbucher brandtbucher approved these changes
ambv Awaiting requested review from ambv

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL