Skip to content

Conversation

@labkey-tchad
Copy link
Member

Rationale

While writing additional tests for editable grids, I made some improvements to FieldInfo and FieldKey that I'd like to roll out before finishing the new editable grid tests.

Related Pull Requests

Changes

  • Make FieldInfo compatible with recent column identifier improvements
  • Add FieldInfo.random to make defining fields with random names less verbose
  • Update EditableGridTest to use FieldInfo
  • Update SampleTypeNameExpressionTest to use random field names
  • Move TestDataGenerator.writeData functionality into TestDataUtils

Copy link
Contributor

@labkey-danield labkey-danield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test shouldn't always quote the parent sample.

Comment on lines 248 to 251
log("Verify import tsv should successfully create derivatives from parent starting with #, as long as values are quoted");
data = "MaterialInputs/" + EscapeUtil.fieldKeyEncodePart(PARENT_SAMPLE_TYPE) + "\n"; // fully encoded
data += "\"" + PARENT_SAMPLE_03 + "\"\n";
data += "\"" + PARENT_SAMPLE_03 + "," + PARENT_SAMPLE_02 + "\"\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't quote the parent id all the time. Having a parent id not quoted is valid input, and this should be tested. Obviously there are exception (if the name begins with a # and it is the first character).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quoted these because PARENT_SAMPLE_03 does start with a #.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I obviously missed that.
I'll mark the PR as approved.

{
return nameExpressionNeedsEscaping.matcher(name).replaceAll("\\\\$1");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

* @param fieldDefinitionMutator will be invoked by {@link #getFieldDefinition()}
* @return a new FieldInfo with the provided mutator. Any existing mutator will be replaced.
*/
@Contract(pure = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious to know the use case for a Consumer of FieldDefinition

expectedValue, actualValue);
}

private String tsvFromColumn(List<String> column)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't suppose this method is generic enough to belong in TestDataUtils?

Copy link
Contributor

@labkey-chrisj labkey-chrisj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm excited for these changes and all the fieldkey work you're doing here. I don't have any requests for changes, will defer to Dan

@labkey-tchad labkey-tchad merged commit 7f9b317 into develop Jun 26, 2025
6 checks passed
@labkey-tchad labkey-tchad deleted the fb_improveFieldInfo branch June 26, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants