Skip to content

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

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 4 commits into from
Jul 13, 2021

Conversation

jsnklln
Copy link
Contributor

@jsnklln jsnklln commented Nov 12, 2019

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

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 brandtbucher added the type-bug An unexpected behavior, bug, or error label Nov 12, 2019
@brandtbucher
Copy link
Member

Thanks @jsnklln!

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

jsnklln commented Nov 12, 2019

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

@mark99i
Copy link

mark99i commented Nov 13, 2019

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

@jsnklln
Copy link
Contributor Author

jsnklln commented Nov 13, 2019

@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
read correctly.  Included a check for key=value because
the original issue mentioned that it might not be working.
@jsnklln
Copy link
Contributor Author

jsnklln commented Nov 13, 2019

@brandtbucher tests have been pushed.

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

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

@jsnklln
Copy link
Contributor Author

jsnklln commented Nov 13, 2019

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

@jsnklln
Copy link
Contributor Author

jsnklln commented Dec 4, 2019

@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 brandtbucher requested a review from ambv February 21, 2020 04:21
@brandtbucher
Copy link
Member

Closing and reopening to trigger CI.

@brandtbucher
Copy link
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 ambv merged commit 2924bb1 into python:main Jul 13, 2021
@ambv ambv added the needs backport to 3.10 only security fixes label Jul 13, 2021
@miss-islington
Copy link
Contributor

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

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 13, 2021
@bedevere-bot
Copy link

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
…H-17129)

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

Co-authored-by: jsnklln <[email protected]>
@ambv
Copy link
Contributor

ambv commented Jul 13, 2021

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

ambv pushed a commit that referenced this pull request Jul 13, 2021
… (#27110)

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

Co-authored-by: jsnklln <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants