joutvhu / fixed-width-parser

Fixed Width Parser: parse fixed width string to object and export object to fixed width string
MIT License
15 stars 3 forks source link

Feed unpadded strings to validation on write #24

Closed f4lco closed 3 years ago

f4lco commented 3 years ago

When writing, I think the validation routine should work on unpadded strings. This has two reasons:

Validation stays the same

When

Only this way the input to the validation stays the same, because it consists of the string without padding.

Avoids having the padding character as part of the format

Below tests would also pass if the padding was part of the format or option string, as in

@FixedOption(options = {"blue ", "white"})
private String color;

But this would require changing the options or the date format every time the padding requirements change (padding character, padding length) which is not desired. Adding the padding as part of the format feels like a hack and escapes the control of FixedField#padding, for example.

Thank you for taking the time 👍

f4lco commented 3 years ago

@joutvhu what's your opinion on this? Can we follow up on this PR? Thanks.

joutvhu commented 3 years ago

Writer: pad is required (You have to fill something in the string to make it long enough, or cut it off if it's too long)

Reader: trim is optional (You can use @FixedField.keepPadding to decide whether to keep padding or not)

f4lco commented 3 years ago

@joutvhu thank you for your reply. After a fair bit of pondering, I see my change cannot fulfil the following test case:

@Test
void optionsWithPadding() {
    Stone stone = new Stone();
    stone.color = "blue";

    assertEquals("blue ", fixedParser.export(stone));
    assertEquals("blue ", fixedParser.parse(Stone.class, "blue ").color);
}

@FixedObject
@Data
public static class Stone {
    @FixedField(length = 5, keepPadding = KeepPadding.KEEP)
    @FixedOption(options = {"blue ", "white"})
    private String color;
}

No matter which values I put into @FixedOption, either the reading or the writing validation fails. That helped me see what you meant with "pad is required / trim is optional". Thank you for your expertise & and for introducing the proper fix on the development branch!

That makes me think: KeepPadding.KEEP also seems then to be a good candidate for an additional test case, just to avoid future regressions. I updated this PR with the new test case, please have a look.