Skip to content
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

Allow checks based on values #1

Closed

Conversation

mitchellrj
Copy link

This pull request adds the ability to perform checks based on:

  • Attribute deletion as opposed to setting
  • New values to be assigned to attributes if the check passes
  • Operands to in-place mutation operations (__iadd__, __imul__, etc) which would normally be called with check rather than check_getattr.

It achieves this by adding a new interface with the following extra check methods which are called by the proxy objects if provided:

  • check_setattr_with_value: for checks based on values to be assigned to attributes. Signature is as check_setattr except with an extra value parameter.
  • check_with_value: for checks based on operands of in-place mutation operations. Signature is as check except with an extra value parameter.
  • check_delattr: for checks based on attribute deletion rather than mutation.

…tes, deletion of attributes and operands of in-place mutation operations.
@mitchellrj
Copy link
Author

(I have signed the Zope contributor agreement already)

@mitchellrj
Copy link
Author

@tseaver any input?

r = PyObject_CallMethodObjArgs(self->proxy_checker, meth,
self->proxy.proxy_object, name,
NULL);
if (value != NULL
Copy link
Member

Choose a reason for hiding this comment

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

A couple of style preferences here:

  • Please enclose even single-statement suites in braces (FBO later edits by indenttion-centric Python developers).
  • Can you add / fix comments for each suite? I think the one in line #187 is really for line #203.

@tseaver
Copy link
Member

tseaver commented Oct 15, 2013

Other than the inline comment, the PR looks good to me. I don't think the performance impact is going to be important (but I guess I could be wrong).

@mitchellrj
Copy link
Author

Thanks! Should all be sorted now. Ready for merge / release whenever.

@tseaver
Copy link
Member

tseaver commented Oct 29, 2013

It looks good to me. I'm going to ask @jimfulton to review for performance impact, as I don't actually use the checkers in production.

@ghost ghost assigned jimfulton Oct 29, 2013
@mitchellrj
Copy link
Author

@tseaver @jimfulton bump

@mitchellrj
Copy link
Author

I have now lost access to the repository from which this pull request was made. If it needs rebasing, this will have to be done elsewhere.

@mgedmin
Copy link
Member

mgedmin commented May 6, 2014

I pushed your changes to the mitchellrj-extended-setattr-behavior branch in this repo so they don't get lost.

@tseaver
Copy link
Member

tseaver commented May 24, 2017

Superseded by PR #25.

@tseaver tseaver closed this May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants