Orthogonal DateTimePeriod implementation #80
Conversation
|
|
||
| @Test | ||
| fun parseIsoString() { | ||
| assertEquals(DateTimePeriod(years = 1), DateTimePeriod.parse("P1Y")) |
There was a problem hiding this comment.
Better to assert the expected property values rather than equality as a whole in this test. Because otherwise it's unclear what values these properties would get as a result of parsing.
There was a problem hiding this comment.
You raise a good point, though I think that the values of properties and the results of parsing are separate concerns, so I added another test to check the normalization.
| assertEquals(DateTimePeriod(nanoseconds = -1L), DateTimePeriod.parse("-PT0.000000001S")) | ||
| assertEquals(DateTimePeriod(days = 1, nanoseconds = -1L), DateTimePeriod.parse("P1DT-0.000000001S")) | ||
| assertEquals(DateTimePeriod(seconds = -1, nanoseconds = 1L), DateTimePeriod.parse("-PT0.999999999S")) | ||
| assertEquals(DateTimePeriod(days = 1, seconds = -1, nanoseconds = 1L), DateTimePeriod.parse("P1DT-0.999999999S")) |
There was a problem hiding this comment.
A couple of negative tests won't hurt. For example that DatePeriod.parse fails on non-zero time components.
| @@ -12,6 +12,39 @@ import kotlin.time.* | |||
|
|
|||
| class DateTimePeriodTest { | |||
|
|
|||
| @Test | |||
| fun normalization() { | |||
| assertPeriodComponents(DateTimePeriod(years = 1) as DatePeriod, years = 1) | |||
There was a problem hiding this comment.
Can you use this assertion function in tests for parse as well?
There was a problem hiding this comment.
Sure, but, in my opinion, it would make those tests less clear. For example,
assertEquals(DateTimePeriod(months = 14), DateTimePeriod.parse("P14M"))clearly implies that DateTimePeriod(months = m) == DateTimePeriod.parse("P${m}M"), which is a property that we want. Seeing the result in terms of values of class properties would obscure this. Moreover, the values of class properties are now being tested with normalization().
What benefits do you see in using this function for tests of parsing?
There was a problem hiding this comment.
By mixing parsing and normalized equality this test makes an impression that the resulting properties of the parsed DateTimePeriod would be the same as those passed to the constructor-like function of expected value. This is misleading.
There was a problem hiding this comment.
For some unfathomable reason, I didn't see a notification about your response. I am sorry for the delay.
I still disagree and consider these concerns orthogonal: we want parsing to behave the same way construction does, and we want construction to produce specific values of properties. Parsing is not semantically connected to the values of properties, and mixing these rubs me the wrong way. However, I don't consider this issue important enough to die on this hill, so I added a commit to accommodate your preferences, though I would be glad to revert it were you to change your mind about this.
There was a problem hiding this comment.
As a compromise, we could leave the equality assertions in the parsing test, but add assertPeriodComponents for those values that cause normalization to happen. This would be done mostly for explanatory reasons.
There was a problem hiding this comment.
I added checks for actual values of properties to places where normalization does affect them.
Now, it only has three components: months, days, and nanoseconds, and all the other properties are just representations of these ones. This way, for each DateTimePeriod there exists a well-defined ISO-8601 representation, and `toString()` behaves correctly. Fixes #79
Co-authored-by: ilya-g <[email protected]>
dkhalanskyjb
force-pushed
the
orthogonal-dateTimePeriod
branch
from
1b1e33c to
3214ee3
Compare
dkhalanskyjb
force-pushed
the
orthogonal-dateTimePeriod
branch
from
a8d3aac to
5d7fb04
Compare
No description provided.