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

fixed builtin method overriding by throwing key error and checking for `... #14

Closed
wants to merge 6 commits into from

Conversation

jxnl
Copy link

@jxnl jxnl commented Dec 13, 2014

Not sure about the prefered style but I've added two tests

…r `BuiltinMethodType`

reduce dict_keys to list for 2.7+
  fixed builtin method overriding by throwing key error and checking for `BuiltinMethodType`
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.52%) when pulling 439820a on jxnl:master into 3dcff2a on mewwts:master.

@frankcash
Copy link

👎

By preventing stator from overwriting values, getattr comes from super() so it does not need a check.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4de8c87 on jxnl:master into 3dcff2a on mewwts:master.

@mewwts
Copy link
Owner

mewwts commented Dec 13, 2014

Hey @jxnl, thanks for contributing!
I'd prefer the import to be on the top of the file, as we don't want to import everytime an attribute is being set. Also, I'd prefer to leave the print out, and change the KeyError to an AttributeError as to be consistent with a dict:

>>> a = dict()
>>> a.keys = 2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'dict' object attribute 'keys' is read-only

As for the tests, just some spacing in the dict-literals, i.e. {'a': 3} instead of {'a':3}. Otherwise I think it looks good.

Also, I don't know, but I feel there might be a different way to solve this. Anyways, thanks a bunch!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8d4513e on jxnl:master into 3dcff2a on mewwts:master.

@jxnl
Copy link
Author

jxnl commented Dec 13, 2014

I think it has to do with setattr changing __dict__ versus setting an item in the Dict...

I know that the attribute dictionary behaves differently than a normal dict so I don't know if we should have the setattr from super() throw the AttributeError since we are actually redirecting the setattr to setitem.

Would it make sense to change __getattr__ to try super().__getattr__ before self.__getitem__

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 48f1ebb on jxnl:master into 3dcff2a on mewwts:master.

@mewwts
Copy link
Owner

mewwts commented Dec 13, 2014

Yes, if we never let an attribute be defined (we redirect those into getting an item), we could simply check if we already have a property (which is bound to be a dict-property), and throw an attributeerror?

EDIT: dicts don't have a getattr obviously!

@mewwts
Copy link
Owner

mewwts commented Dec 13, 2014

Hey @jxnl, take a look at ec26acb, this should solve it right?

@jxnl
Copy link
Author

jxnl commented Dec 13, 2014

i'd think so!

@mewwts
Copy link
Owner

mewwts commented Dec 13, 2014

I really appreciate this @jxnl! 👍 But I feel bad for not merging your PR, I just got so excited I had to burst out and commit it. Hope you'll continue to invest in this!

@jxnl
Copy link
Author

jxnl commented Dec 13, 2014

no worries :)

@jxnl jxnl closed this Dec 13, 2014
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.

None yet

4 participants