Skip to content
This repository has been archived by the owner. It is now read-only.

Contains/Not Contains throws wrong errors #877

Merged
merged 10 commits into from Aug 9, 2018

Conversation

@patiences
Copy link
Contributor

@patiences patiences commented Jul 27, 2018

  1. in/__contains__ throws the wrong error on non-iterable types (like int) -- should be TypeError not AttributeError
  2. not in/__not_contains__ should probably use __contains__ instead of looking for __not_contains__ attribute
  3. org.python.types.Str.contains should return a org.python.types.Bool

This PR makes the following changes:

  1. Addresses the problems above
  2. Removes the __not_contains__ attribute from org/python/Objects. I verified that each __not_contains__ attribute that was defined was just an inverse of its __contains__ attribute. Since the operator not in is defined to have the inverse true value of in (from https://docs.python.org/3/reference/expressions.html#membership-test-operations), there is no need to define separate __not_contains__ attributes, they should all direct to __contains__. See org/python/types/Object.java#not_contains.
@patiences patiences changed the title Contains/Not Contains throws wrong errors [WIP] Contains/Not Contains throws wrong errors Jul 27, 2018
@patiences patiences changed the title [WIP] Contains/Not Contains throws wrong errors Contains/Not Contains throws wrong errors Jul 30, 2018
@@ -552,14 +552,6 @@ public void __delitem__(org.python.Object index) {
throw new org.python.exceptions.AttributeError(this, "__contains__");
}
Copy link
Contributor Author

@patiences patiences Jul 30, 2018

I'm not sure that this AttributeError is strictly necessary -- seems like a __contains__ call is either successful (it's defined elsewhere) or a TypeError is thrown (and this is constructed after this AttributeError is thrown and caught). Could this be replaced by org.python.types.NotImplemented?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

👍

@freakboy3742 freakboy3742 merged commit 2262a71 into beeware:master Aug 9, 2018
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants