Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-103384: Generalize the regex patternBaseConfigurator.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vsajip merged 22 commits intopython:mainfromdhuadaar:gh-103384
Aug 25, 2023

Conversation

@dhuadaar
Copy link
Contributor

@dhuadaardhuadaar commentedApr 9, 2023•
edited by bedevere-bot
Loading

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

@dhuadaardhuadaar requested a review fromvsajip as acode ownerApril 9, 2023 05:25
@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@ghost
Copy link

ghost commentedApr 9, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@tomasr8
Copy link
Member

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

@dhuadaar
Copy link
ContributorAuthor

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.

tomasr8 reacted with thumbs up emoji

@tomasr8
Copy link
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.

dhuadaar reacted with thumbs up emoji

@dhuadaar
Copy link
ContributorAuthor

dhuadaar commentedApr 9, 2023•
edited
Loading

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.

tomasr8 reacted with thumbs up emoji

Copy link
Member

@vsajipvsajip 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 phraseI 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.

@dhuadaardhuadaar changed the titlegh-103384: Suggested change Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex patternBaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.gh-103384: Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex patternBaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.Apr 13, 2023
…AV7iB.rstCo-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
@dhuadaar
Copy link
ContributorAuthor

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

@dhuadaardhuadaar changed the titlegh-103384: Fix regex pattern for BaseConfigurator.INDEX_PATTERN Generalize the regex patternBaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.gh-103384: Generalize the regex patternBaseConfigurator.INDEX_PATTERN to allow spaces and non-alphanumeric characters in keys.Apr 13, 2023
@dhuadaar
Copy link
ContributorAuthor

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

@vsajip
Copy link
Member

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

tomasr8 reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

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

dhuadaar reacted with thumbs up emoji

@vsajipvsajip merged commit8d40520 intopython:mainAug 25, 2023
@erlend-aasland
Copy link
Contributor

Should this be backported?

@vsajip
Copy link
Member

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

erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

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

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

Reviewers

@arhadthedevarhadthedevarhadthedev left review comments

@vsajipvsajipvsajip approved these changes

@tomasr8tomasr8Awaiting requested review from tomasr8

@hugovkhugovkAwaiting requested review from hugovk

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@dhuadaar@bedevere-bot@tomasr8@vsajip@erlend-aasland@hugovk@arhadthedev@AlexWaygood
[8]ページ先頭
©2009-2025 Movatter.jp