bugfix AbstractTuple.getList and copy constructor new KeyValueTuple(tuple)

[mswertz]

note that second commit message is misleading. Should have been added copy constructor KeyValueTuple(tuple) and KeyValueTuple.set(tuple)

[mswertz]

Please merge.

[dennishendriksen]

Consistency: Apply AbstractTuple.getList changes to all get methods Request: Write tests for TupleUtils

Discussion: I don't understand the use of TupleUtils methods, can you explain? Why should Tuple know anything about freemarker templates? Should we move TupleUtils to org.molgenis.util since it is a utility class and not directly io related?

[mswertz]

In my mind the .io package is independent of rest of MOLGENIS, and so could be any helper method that helps classes in .io. So I would prefer it to be part of *.io package (to become a seperate mave module), optionally in a sub package.

Purpose of the freemarker dependency is that in compute we use freemarker templates to calculate the values (i.e. you only have to set a few values, all the others are derived). This actually is quite a powerful feature. See the 'solve()' method.

I agree tests would be useful and I will add them in due course but is that a reason to wait with the merge?

[mswertz]

BTW I don't understand the comment "Consistency: Apply AbstractTuple.getList changes to all get methods". Because if you say 'toString' you expect it to succeed, and toInt() you expect it to fail, which it does?

[dennishendriksen]

With my suggestion I meant applying your getList fix to the other get methods in AbstractTuple. For example:

@Override public Timestamp getTimestamp(String colName) { Object obj = get(colName); if(obj == null) return null; else if(obj instanceof Timestamp) return (Timestamp) obj; else { String str = getString(colName); return str != null ? Timestamp.valueOf(str) : null; } }

Do you agree? Would be nice to have a test for your fix in AbstractTupleTest by the way. If you don't have time, I can also make these changes directly after merge.

[mswertz]

What is the motivation?

Reason for the change in getList() was that the value actually changed if you call

List list1 = tuple.getList(..); tuple.set(list1); if( !list1.equals(tuple.getList(..)) { ; //returned false! }

Question: are there similar problems with the other getXXX as well?

M

On Tue, Jan 22, 2013 at 9:52 AM, Dennis Hendriksen <notifications@github.com

wrote:

With my suggestion I meant applying your getList fix to the other get methods in AbstractTuple. For example:

@Override https://github.com/Override public Timestamp getTimestamp(String colName) { Object obj = get(colName); if(obj == null) return null; else if(obj instanceof Timestamp) return (Timestamp) obj; else { String str = getString(colName); return str != null ? Timestamp.valueOf(str) : null; } }

Do you agree? Would be nice to have a test for your fix in AbstractTupleTest by the way.

— Reply to this email directly or view it on GitHubhttps://github.com/molgenis/molgenis/pull/207#issuecomment-12533917.

[dennishendriksen]

I'm wondering if the compute app isn't a better location for Freemarker specific Tuple manipulation? About the io package contents: my opinion is that all packages in molgenis core can depend on the utils package and the utils package does not depend on the other packages. This is for example the structure used in the java API.

I agree tests would be useful and I will add them in due course but is that a reason to wait with the merge?

I disagree. If people really need these changes, they can pull from your fork.

[dennishendriksen]

What is the motivation?

tuple.set("x", new Timestamp(...); tuple.getTimestamp("x") <-- fails

[mswertz]

Clear: so we need tests of this form for all getters/setters !

M

On Tue, Jan 22, 2013 at 10:03 AM, Dennis Hendriksen < notifications@github.com> wrote:

What is the motivation? tuple.set("x", new Timestamp(...); tuple.getTimestamp("x") <-- fails

— Reply to this email directly or view it on GitHubhttps://github.com/molgenis/molgenis/pull/207#issuecomment-12534201.

[mswertz]

(1) I have no real problem with keeping TupleUtils in compute for a while. But at some point it should be part of molgenis core.

I don't like to have seperate TupleXxxUtils because then there is big chance of duplicated implementations.

(2) Then we can discuss what should go together in a maven module. Currently dependency graph would be

util <- io

I would prefer to put 'util' and 'io' into one maven module to keep the numbers of maven modules manageable.

M

On Tue, Jan 22, 2013 at 10:02 AM, Dennis Hendriksen < notifications@github.com> wrote:

I'm wondering if the compute app isn't a better location for Freemarker specific Tuple manipulation? About the io package contents: my opinion is that all packages in molgenis core can depend on the utils package and the utils package does not depend on the other packages. This is for example the structure used in the java API.

I agree tests would be useful and I will add them in due course but is that a reason to wait with the merge? I disagree. If people really need these changes, they can pull from your fork.

— Reply to this email directly or view it on GitHubhttps://github.com/molgenis/molgenis/pull/207#issuecomment-12534149.

[dennishendriksen]

I would prefer to put 'util' and 'io' into one maven module to keep the numbers of maven modules manageable.

Agreed

[mswertz]

okay. Having said that I will move TupleUtils to 'util' for now. So don't merge just yet :-)

M

On Tue, Jan 22, 2013 at 10:10 AM, Dennis Hendriksen < notifications@github.com> wrote:

I would prefer to put 'util' and 'io' into one maven module to keep the numbers of maven modules manageable.

Agreed

— Reply to this email directly or view it on GitHubhttps://github.com/molgenis/molgenis/pull/207#issuecomment-12534338.