Issue 53431: Data Class and Sample Type data doesn't round-trip via folder export/import for field names with special char#6840
Conversation
…older export/import for field names with special char - use ColumnHeaderType.ImportField instead of FieldKey for writing folder archive tsv data column headers
| try (TSVGridWriter writer = new TSVGridWriter(plateQueryView::getResults, displayColumns, Collections.singletonMap(sampleIdNameFieldKey.toString(), "Sample ID"))) | ||
| { | ||
| writer.setDelimiterCharacter(delim); | ||
| writer.setColumnHeaderType(ColumnHeaderType.FieldKey); |
There was a problem hiding this comment.
My main question is about the usage of these different ColumnHeaderType. Reading through the comments that surround them I'm still not sure which one to choose in which situation.
It looks like up to this point all the usages of ImportField align with usage in import templates, however, there are other usages of ColumnHeaderType.FieldKey that would seem to align with what you're using them for here.
Is ImportField just the new and improved FieldKey that we should be using more widely (I assume not)? I may be off-base here as you may have already discussed with @labkey-susanh who looks to familiarity with this enumeration.
There was a problem hiding this comment.
Yeah, I agree. This enum is confusing. The comment for FieldKey, which has been there since 2015 saying "Useful for import/export round-tripping" is wrong. We do not support FieldKey on import (see my attempt to add it here #6270). Maybe it is time to remove that part of the comment.
There was a problem hiding this comment.
ImportField was added specifically because, as Cory said, round-tripping with FieldKey doesn't work currently. Given this, I'm not sure when FieldKey is useful as a column header type. Perhaps it worked at some point, but I doubt it. Since this targets 25.7, we shouldn't remove it altogether here, I think, but might consider that for develop.
…/export round-tripping" since that isn't the case
| try (TSVGridWriter writer = new TSVGridWriter(plateQueryView::getResults, displayColumns, Collections.singletonMap(sampleIdNameFieldKey.toString(), "Sample ID"))) | ||
| { | ||
| writer.setDelimiterCharacter(delim); | ||
| writer.setColumnHeaderType(ColumnHeaderType.FieldKey); |
There was a problem hiding this comment.
ImportField was added specifically because, as Cory said, round-tripping with FieldKey doesn't work currently. Given this, I'm not sure when FieldKey is useful as a column header type. Perhaps it worked at some point, but I doubt it. Since this targets 25.7, we shouldn't remove it altogether here, I think, but might consider that for develop.
cnathe
added
code review complete
TeamCity test review complete
and removed
needs code review
needs TeamCity test review
labels
cnathe
added
needs TeamCity test review
and removed
TeamCity test review complete
labels
3 participants
Rationale
https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=53431
Related Pull Requests
Changes
Tasks