ResponseProcessor State-setting and Tests #178
Conversation
… `Provenancable` through provenance and back to check whether the produce the same object. Wrote a `structuralEquals` method on `ConfigurationData` to support it.
…anged the logic in `FieldResponseProcessor.postConfig` to properly handle all cases.
Craigacp
added
Oracle employee
| * and provenance representations are the same using {@link ConfigurationData#structuralEquals(List, List, String, String)}. | ||
| * @param itm The object whose equality is to be tested | ||
| */ | ||
| public static <P extends ObjectProvenance, C extends Configurable & Provenancable<P>> void testConfigurableRoundtrip(C itm) { |
There was a problem hiding this comment.
This type bound could be P extends ConfiguredObjectProvenance. Not sure if that limits its functionality any.
| @@ -76,6 +81,27 @@ public static ImmutableFeatureMap mkFeatureMap(String... features) { | |||
| return ex; | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
There are two blank lines between this function and the surrounding ones, but we usually have one.
| defaultValues = defaultValues == null ? Collections.nCopies(fieldNames.size(), defaultValue) : defaultValues; | ||
| } else if (fieldNames != null) { // multiple fields | ||
| if (defaultValues != null && defaultValues.size() == fieldNames.size()) { // check first for multiple defaults | ||
| // this is the default case, nothing needs to be done |
There was a problem hiding this comment.
Can we flip this around to get rid of the empty if branch? Maybe start with defaultValues == null && defaultValue == null to catch the else case, and then have the defaultValue != null one, then defaultValues != null && defaultValues.size() != fieldNames.size() to throw PropertyException.
There was a problem hiding this comment.
I agree that the empty branch is awkward, but I think this is actually a fair bit more readable. An earlier version without it had bugs that were almost impossible to identify by inspection. However, I'll write documentation for this method and its partner in BinaryResponseProcessor that explains the logic at play.
`BinaryResponseProcessor` to be more readable.
| @@ -75,34 +75,42 @@ | |||
| @ConfigurableName | |||
| private String configName; | |||
|
|
|||
| /** | |||
| * We support specifying field names and default values both singly through {@link #fieldName} and {@link #positiveResponse} | |||
| * and in a list through {@link #fieldNames} and {@link #positiveResponses}. Canonically all internal logic is driven by | |||
There was a problem hiding this comment.
This is a weird javadoc comment as it's more for our benefit as developers rather than anyone who uses this class, so it probably should be a regular comment at the start of this method then it doesn't show up in the docs.
| @@ -60,35 +60,41 @@ | |||
| @ConfigurableName | |||
| private String configName; | |||
|
|
|||
| /** | |||
| * We support specifying field names and default values both singly through {@link #fieldName} and {@link #defaultValue} | |||
There was a problem hiding this comment.
Same comment as before. At minimum this should not that the method should not be called by user code as the default postConfig javadoc says.
Description
Fixes an issue with multi-field
ResponseProcessorinstances where the end state of the produced object, when written back out into a configured/provenanced form and re-instantiated wouldn't be processed correctly by thepostConfigcall. Also added a test for roundtripping any type that is bothConfigurableandProvenanceablethrough their respective forms to ensure that the end results are structurally equal according to theConfigurationData#structuralEqualsmethod in OLCUT, and tests for this behavior inBinaryResponseProcessorandFieldResponseProcessor.This change depends on OLCUT v5.2.0, and thus requires updating Tribuo to depend on that OLCUT version.