fix #374 updating method logic to address several issues #375
Conversation
|
|
||
| private StringStatement createAddOrSetIndex(String op, String propertyName, String returnType) { | ||
| return new StringStatement( | ||
| "if (index < 0 || index >= " + propertyName + ".size()) { _visitables.get(\"" + propertyName |
There was a problem hiding this comment.
Can you please elaborate here?
Why do we need fallbacks? Shouldn't we just let it fail instead?
There was a problem hiding this comment.
Why do we need fallbacks? Shouldn't we just let it fail instead?
The fallbacks, via builderOf, handles the failure case. Several these methods had the same problem as #350
There was a problem hiding this comment.
I am still not sure I follow. Can you please further elaborate?
Given that we also have an additional topic regarding Editables in #376 it might make sense to schedule a meeting or something.
There was a problem hiding this comment.
I am still not sure I follow. Can you please further elaborate?
The existing implementation of setToCollection(index, interface) looks like:
if (item instanceof Concrete1) { setToConcrete...
if (item instanceof Concrete2) { setToConcrete...
return (A)this;
So if the item is a not a known buildable type it will simply fall through and do nothing - which is what was observed with #350
This change makes setTo and addTo have the else case
} else { VisitableBuilder builder = builderOf(item) ... }
So that if something has it builder it will succeed, or if not there will be an exception.
| @@ -744,12 +740,20 @@ public List<Method> apply(final Property property) { | |||
|
|
|||
| addSingleItemAtIndex = new MethodBuilder(addSingleItemAtIndex).withParameters(parameters).editBlock() | |||
| .withStatements(createAddToDescendants("addTo", descendants, true), | |||
| new StringStatement("return (" + returnType + ")this;")) | |||
There was a problem hiding this comment.
There is a method available caleld createAddToDescendantsFallback maybe this should be used instead?
It seems as createAddToDescendantsFallback was meant to do the same thing but somehow got forgotten.
There was a problem hiding this comment.
There is a method available caleld createAddToDescendantsFallback maybe this should be used instead?
To clarify there are two things happening. One is usage of builderOf, the other is the actual operation. createAddToDescendantsFallback is written to do both, so it's not applicable to the other operations - indexed addTo, setTo, remove.
Note that not everywhere createAddOrSetIndex is a fallback case - the same logic applies to both fallback https://github.com/sundrio/sundrio/pull/375/files#diff-f95a091f7367292c3d8e6c10eb03b6411bfe67573270d10bb9393f947dc5ec54R755 and regular logic https://github.com/sundrio/sundrio/pull/375/files#diff-f95a091f7367292c3d8e6c10eb03b6411bfe67573270d10bb9393f947dc5ec54R773 - this does ensure that consistent indexing is used across the methods, which is also called out in #374
iocanel
added
the
downstream-check
No description provided.