Skip to content

Safer GetAttr(name, default) #1578

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 1 commit into from
Oct 1, 2021
Merged

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Oct 1, 2021

What does this implement/fix? Explain your changes.

Prior to this change PyObject.GetAttr(name, default) would ignore any exceptions, including ones unrelated to missing attribute. For example (hypothetically) PyObject is a file, and you call file.GetAttr("len", 0), it would return 0 on I/O error even though file object actually has len attribute.

Remarks

I marked the method obsolete, as even now when we restrict ignored errors to AttributeError and derived types, the behavior of this method still going to be misleading in some scenarios. For example,

class RaisesAttrErr:
  @property
  def prop(self): raise AttributeError

class OK:
  def __init__(self, impl):
    self._impl = impl

  @property
  def existing_prop(self): return self._impl.prop
PythonEngine.Eval("OK(RaisesAttrErr())").GetAttr("existing_prop", 0)
// returns 0 despite existing_prop actually existing -^ in OK

Does this close any currently open issues?

fixes #1036

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • Updated the CHANGELOG

@lostmsu lostmsu merged commit 5d0d01a into pythonnet:master Oct 1, 2021
@lostmsu lostmsu deleted the get-attr-default branch October 1, 2021 20:07
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.

PyObject.GetAttr(string, PyObject _default) masks any exceptions
2 participants