ExpObjectDataIterator: consolidate iterator logic #6788
Conversation
| try | ||
| { | ||
| if (_isSample) | ||
| ExpObject expObject = null; | ||
| if (_context.getInsertOption().mergeRows && _nameCol != null) |
There was a problem hiding this comment.
I know this isn't your doing, but it's not clear why this logic about the name column is inside the if for the lsid being a string. The lsid comes into play only if the object isn't found by name.
There was a problem hiding this comment.
Yikes. I agree, this logic was pretty snarled. I've refactored this to not be nested in the lsid check. See 7cb5ff1.
experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java
Outdated
Show resolved
Hide resolved
labkey-nicka
force-pushed
the
fb_alias_merge
branch
from
6c62e5d to
e694ba8
Compare
labkey-nicka
changed the title
| } | ||
| } | ||
| } | ||
| else if (_lsidCol != null) |
There was a problem hiding this comment.
I know it's kind of silly, but you could have both the name and the LSID columns, so perhaps this logic should be more like in the FlagDataIterator where we go into the lsid block of obj is null?
| { | ||
| for (Map.Entry<String, Object> entry : _lsidAliasMap.entrySet()) | ||
| if (_context.getInsertOption().mergeRows && _nameCol != null) |
There was a problem hiding this comment.
I'm not sure the check for mergeRows here is correct. Couldn't you also have a _nameCol for the update case?
|
|
||
| Object flagValue = get(_flagCol); | ||
| ExpObject expObject = null; | ||
| if (_nameCol != null && _context.getInsertOption().mergeRows) |
There was a problem hiding this comment.
Same question here about merge vs. update.
Rationale
This is the remanent refactor of prior work done to address Issue 53153.
Related Pull Requests
Changes
ExpDataTypeDataIteratorto encapsulate logic for data iterators that operate on experiment materials and data.FlagDataIteratorlogic to not rely on presence oflsidcolumn