Skip to content

gh-103384: Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.#103391

Merged
vsajip merged 22 commits into
python:mainfrom
dhuadaar:gh-103384
Aug 25, 2023
Merged

gh-103384: Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.#103391
vsajip merged 22 commits into
python:mainfrom
dhuadaar:gh-103384

Conversation

@dhuadaar

@dhuadaar dhuadaar commented Apr 9, 2023

Copy link
Copy Markdown
Contributor

The issue described a broken regex pattern in the logging module.
Fixed the regex and added tests for the same.

@dhuadaar dhuadaar requested a review from vsajip as a code owner April 9, 2023 05:25
@bedevere-bot

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost

ghost commented Apr 9, 2023

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

Comment thread Lib/test/test_logging.py Outdated
Comment thread Lib/test/test_logging.py Outdated
Comment thread Lib/logging/config.py Outdated
@tomasr8

tomasr8 commented Apr 9, 2023

Copy link
Copy Markdown
Member

I think a test with some special characters would also be useful :)

@dhuadaar

dhuadaar commented Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

I think a test with some special characters would also be useful :)

Makes sense.

Thanks for the suggestions. I will add some of those combinations.

@dhuadaar dhuadaar requested review from hugovk and tomasr8 April 9, 2023 09:43
@tomasr8

tomasr8 commented Apr 9, 2023

Copy link
Copy Markdown
Member

I'd also add tests for some edge cases like the empty string (if we want to allow that) and multiple nesting levels e.g. [x][y] or even [x].y[z] to make sure the regex is working as expected.

Comment thread Lib/test/test_logging.py Outdated
@dhuadaar

dhuadaar commented Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

I'd also add tests for some edge cases like the empty string (if we want to allow that) and multiple nesting levels e.g. [x][y] or even [x].y[z] to make sure the regex is working as expected.

I have added a new nested dictionary to the existing template we use in the test. Added new assertions specifically for the nested cases.

Do you suggest making the cases like the following to the part of the new key added?

'nest2': ['k', ['l', 'm'], 'n'],

Update: Added this anyways.

@vsajip vsajip 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.

Thanks for the PR!

Comment thread Lib/logging/config.py Outdated
Comment thread Misc/NEWS.d/next/Library/2023-04-09-05-30-41.gh-issue-103384.zAV7iB.rst Outdated
@bedevere-bot

Copy link
Copy Markdown

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.

@dhuadaar dhuadaar changed the title gh-103384: Suggested change Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys. gh-103384: Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys. Apr 13, 2023
…AV7iB.rst

Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
Comment thread Misc/NEWS.d/next/Library/2023-04-09-05-30-41.gh-issue-103384.zAV7iB.rst Outdated
@dhuadaar

Copy link
Copy Markdown
Contributor Author

@vsajip

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

@vsajip: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from vsajip April 13, 2023 18:18
@dhuadaar dhuadaar changed the title gh-103384: Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys. gh-103384: Generalize the regex pattern BaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys. Apr 13, 2023
@dhuadaar

dhuadaar commented May 2, 2023

Copy link
Copy Markdown
Contributor Author

@vsajip
Could you review this PR? I have made the changes requested.

@vsajip

vsajip commented Aug 4, 2023

Copy link
Copy Markdown
Member

Sorry I've not had time to look at this. Hope to get to it soon.

@erlend-aasland

Copy link
Copy Markdown
Contributor

I'll leave it for the code owner @vsajip to land this :) Thanks for your contribution, @dhuadaar!

@vsajip vsajip merged commit 8d40520 into python:main Aug 25, 2023
@erlend-aasland

Copy link
Copy Markdown
Contributor

Should this be backported?

@vsajip

vsajip commented Aug 25, 2023

Copy link
Copy Markdown
Member

Perhaps - it's arguable whether it's an enhancement or could have been considered a documentation bug.

@erlend-aasland

Copy link
Copy Markdown
Contributor

Ok; if no backport is needed, we should close the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants