Skip to content

gh-94499 Add test for private name mangling in class pattern matching #94500

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, 2022

Conversation

da-woods
Copy link
Contributor

@da-woods da-woods commented Jul 1, 2022

The current status quo is that private attribute names are not
mangled when a class is matched. I've added a test to
document/legitimize this behaviour.

I don't think this needs a news entry (but can create one if required).

I asked about this in Python-dev and it wasn't clear if the current behaviour was by design or not. My view is that it seems reasonable so the right thing to do is to "lock it in" by testing for it, but I'm also happy for

@da-woods da-woods requested a review from brandtbucher as a code owner July 1, 2022 17:46
@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Jul 1, 2022
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 for the test. I think the readability could be a bit improved a bit (it took me a second to figure out what this test was doing)... what do you think of this version?

If you don't like it, at the very least we should change all of the setattrs to normal attribute assignments.

@brandtbucher
Copy link
Member

brandtbucher commented Jul 7, 2022

Ah, I forgot that this entire test is inside of another class, which mangles the c.__attr assignment. Sorry about that. 🤦🏼

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG with one suggestion. I approve of locking in this behavior. @brandtbucher Do you think we should add a clarification to PEP 634?

@brandtbucher
Copy link
Member

brandtbucher commented Jul 12, 2022

Do you think we should add a clarification to PEP 634?

I think the current wording ("The keyword is looked up as an attribute on the subject.") is fine as-is, but I wouldn't reject a PR proposing to modify it.

Nobody really reads PEP 634, though. 🙃

da-woods and others added 4 commits July 13, 2022 11:11
The current status quo is that private attribute names are not
mangled when a class is matched. I've added a test to
document/legimize this behaviour.
@da-woods da-woods force-pushed the pattern-match-mangling branch from 530d483 to 97554c9 Compare July 13, 2022 10:13
@da-woods
Copy link
Contributor Author

Rebased to undo a git mishap! Sorry!

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@gvanrossum gvanrossum merged commit 8d7089f into python:main Jul 13, 2022
@da-woods da-woods deleted the pattern-match-mangling branch July 13, 2022 16:13
@gvanrossum
Copy link
Member

Since it's just a test I don't feel that it needs to be backported, though we could probably backport to 3.11 and 3.10. @brandtbucher What do you think?

@brandtbucher
Copy link
Member

Nah, I’m not worried about this behavior regressing on older branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants