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

MNT: make _setattr_cm more conservative#17620

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

Conversation

tacaswell
Copy link
Member

PR Summary

  • special case methods because they are descriptors
  • fail on any other non-instance attribute

This is a simpler version of#17561

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant

@tacaswelltacaswell added this to thev3.3.0 milestoneJun 11, 2020
@QuLogic
Copy link
Member

CI does not like this at all.

@tacaswell
Copy link
MemberAuthor

Indeed, I thought I ran the tests before I pushed. I must have lost track of what I was doing locally...

@tacaswelltacaswellforce-pushed thesimplified_setattr_cm_method_handling branch from4111ab6 tod0849a6CompareJune 11, 2020 23:03
@tacaswell
Copy link
MemberAuthor

🤦 I edited one working tree and installed and tested a different one.

@tacaswelltacaswellforce-pushed thesimplified_setattr_cm_method_handling branch fromd0849a6 to3b4da04CompareJune 12, 2020 00:01
Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

Not sure what features we should be supporting in the future (re:#17561), but at least this makes it clear what we don't.

origs = {}
for attr in kwargs:
orig = getattr(obj, attr, sentinel)
# monkey patching on a new attribute, this is OK
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps you can reorder this to make the common case first, e.g.

if (attr in obj.__dict__ or orig is sentinel        or isinstance(getattr(type(obj), attr), (property, types.FunctionType)):    origs[attr] = origelse:    raise ValueError(...)

Checking to types.FunctionType in the type is the same as checking for methods, and (as noted in the other PR, I think) won't give a wrong result if an unrelated bound method is assigned as instance var (self.foo = otherobj.method) -- at least I think this point should be fixed.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

That makes sense. I agree the method from another object is a valid concern (sorry I didn't fully register it before).

@tacaswelltacaswellforce-pushed thesimplified_setattr_cm_method_handling branch from6ee399a to756b9c3CompareJune 14, 2020 20:42
@tacaswelltacaswell requested a review fromanntzerJune 14, 2020 20:42
Copy link
Contributor

@anntzeranntzer left a comment

Choose a reason for hiding this comment

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

just style things.

 - special case methods and properties because they are descriptors - fail on any other non-instance attribute
@tacaswelltacaswellforce-pushed thesimplified_setattr_cm_method_handling branch from756b9c3 to9e88fa5CompareJune 15, 2020 15:43
@anntzeranntzer merged commite86e059 intomatplotlib:masterJun 15, 2020
@tacaswelltacaswell deleted the simplified_setattr_cm_method_handling branchJune 15, 2020 18:29
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

3 participants
@tacaswell@QuLogic@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp