Skip to content

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

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 22 commits into from
Aug 25, 2023

Conversation

dhuadaar
Copy link
Contributor

@dhuadaar dhuadaar commented Apr 9, 2023

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

Most changes to Python require a NEWS entry.

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

@ghost
Copy link

ghost commented Apr 9, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@tomasr8
Copy link
Member

tomasr8 commented Apr 9, 2023

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

@dhuadaar
Copy link
Contributor Author

dhuadaar commented Apr 9, 2023

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 tomasr8 and hugovk April 9, 2023 09:43
@tomasr8
Copy link
Member

tomasr8 commented Apr 9, 2023

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.

@dhuadaar
Copy link
Contributor Author

dhuadaar commented Apr 9, 2023

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.

Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@bedevere-bot
Copy link

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

@vsajip

I have made the requested changes; please review again.

@bedevere-bot
Copy link

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

dhuadaar commented May 2, 2023

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

@vsajip
Copy link
Member

vsajip commented Aug 4, 2023

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

@erlend-aasland
Copy link
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
Contributor

Should this be backported?

@vsajip
Copy link
Member

vsajip commented Aug 25, 2023

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

@erlend-aasland
Copy link
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