Inconsistent Vars.addAll

[Mazi-S]

The Vars.addAll method has a default implementation, but is also implemented (overridden) in AbstractVariables.

The interface and class implementations do not match, which can be misleading. Also, the actual implementation in AbstractVariables can behave differently depending on the runtime type of the given argument. In our case it always behaves the same because we only have one class that implements both (Val and Var), although this is misleading.

But a real problem I think we have is this:

Vars<String> vars = Vars.of("a", "b", "c");
Vals<String> vals = Vals.of(Val.of("d"), Val.of("e"), Val.of("f"));

vars.addAll(vals);

vars.at(0).set("x"); // ok
vars.at(3).set("x"); // UnsupportedOperationException

Another related inconsistency is that when vals is created via Vals.of("d", "e", "f") it works and the properties in vals are mutable.

Vars<String> vars = Vars.of("a", "b", "c");
Vals<String> vals = Vals.of("d", "e", "f");

vars.addAll(vals);

vars.at(0).set("x"); // ok
vars.at(3).set("x"); // ok

This is because the properties are created through Var.of (sprouts/impl/AbstractVariables.java:54)

---

I think we should decide what the addAll method should do. I suggest that we stick to the behavior of the other add methods.

• When we get a property (var), we add it.
• When we get a value, we wrap it in a property and add it.
• And no method for adding val's.

For addAll<Vals> it would make sense to add the values wrapped in new properties, but this is not possible because Vals is a sup type of Vars.

[Gleethos]

I agree with your assessment! The fact that a call to set may only work sometimes despite the API claiming that it works is a really big problem.

For addAll it would make sense to add the values wrapped in new properties, but this is not possible because Vals is a sup type of Vars.

Hmm, well it is the other way around. Vals is a supertype. But I get your point here! Either we do not user Vals as parameter, or we introduce 2 types of overloaded methods, one for Vars and one for Vals.

[Mazi-S]

Yes, my last sentence is incorrect. But I think we should not rely on the runtime type to decide what to do, we should rely on the declared type instead.

So we should at least have an addAll(Vars) to be consistent with the other add methods.

And if we decide to have an addAll(Vals) method, we should rely on the declared type:

Vars<String> vars = ...

Vars<String> vars1 = ...
Vals<String> vals1 = ...
Vals<String> vals2 = vars1;

vars.addAll(vars1); // adds the properties
vars.addAll(vals1); // adds the values wrapped in new properties
vars.addAll(vals2); // adds the values wrapped in new properties

And also, if we decide to have an addAll(Vals), why don't we have other add(Val), setAt(int, Val), ..., methods? Would it not be consistent to have them as well? (but maybe that would be too much?)

[Gleethos]

So I would definitely second the first two of your previously made points.

• When we get a mutable property (Var) to a mutable property list (Vars), we simply add it.
• When we get an immutable property (from Val.of...), we wrap it in a new mutable property (Var.of..) and add it.

But I do not fully understand what you mean by:

• And no method for adding val's.

So as you summarized clearly:

vars.addAll(vars1); // adds the properties
vars.addAll(vals1); // adds the values wrapped in new properties
vars.addAll(vals2); // adds the values wrapped in new properties

I would ensure that the addAll method here has the signature addAll(Vals) and internally there is a check for which type of property list it is and then it acts accordingly... And so I don't think it is necessary to have a addAll(Vars) method.

Consequently I would also expect there to be support for Val parameters and implicit conversion... ...as you suggested in your last sentence:

And also, if we decide to have an addAll(Vals), why don't we have other add(Val), setAt(int, Val), ..., methods? Would it not be consistent to have them as well? (but maybe that would be too much?)

The thing we have to acknowledge here is that with the introduction of addAll(Vals) then we do have to do this kind of implicit conversion pf immutable properties to mutable properties. So if we do this, then it follows naturally that there are implicit conversions done for single properties...

So I think we should think really hard about the consequences here to a consumer of the API and if there are pitfalls. I personally do not see any pitfalls or unexpected behaviour. In a sense Vars.of("a","b","c").addAll(Vals.of("d","e","f")) seems to be a less boilerplaty variant of Vars.of("a","b","c").addAll(Vals.of("d","e","f").toVars()), which is not at all confusing or ambiguous...

So in summary, I agree.