[* DateTimeV2] Fix date range (without year) is processed as a single Date with Year (#2850)#2907
Conversation
|
could you please merge this |
| "Resolution": { | ||
| "values": [ | ||
| { | ||
| "timex": "(XXXX-11-19,XXXX-11-20,P1D)", |
There was a problem hiding this comment.
Resolution is incorrect? Should be P2D start in 19 (inclusive) and ends in 21 (not inclusive).
There was a problem hiding this comment.
Is this case different from some other behaviour? We may have a breaking problem here.
There was a problem hiding this comment.
I believe it's another big topic - the date range resolutions are messed up,
there is no clear rule when the last day/week/month is inclusive or not.
There was a problem hiding this comment.
This is how the parsing of this pattern is currently implemented.
There are similar cases in the specs that follow the same logic, for example:
- "I'll be out from 4-23 in next month" -> (2016-12-04,2016-12-23,P19D)
- "I'll be out between 3 and 12 of Sept hahaha" -> (XXXX-09-03,XXXX-09-12,P9D)
- "APEC will happen in Korea on November 18-19" -> (XXXX-11-18,XXXX-11-19,P1D)
- "9 to 12 of June: another tapa festival" -> (XXXX-06-09,XXXX-06-12,P3D)
There was a problem hiding this comment.
I see. Some could be considered ambiguous, but some seem clearly incorrect. Let's not modify for now though, as this is somewhat of a breaking change.
| "Resolution": { | ||
| "values": [ | ||
| { | ||
| "timex": "(XXXX-11-19,XXXX-11-20,P1D)", |
There was a problem hiding this comment.
Incorrect resolution. Text clearly implies P2D.
There was a problem hiding this comment.
Should we modify this behavior?
But it will affect other existing test cases (like the ones pointed above)
There was a problem hiding this comment.
It would be great to add a flag if the last day must be included.
Because at this moment extra day is added for months and years (I will be off in April -> ends 1st of May)
But for weeks and days it’s not included (April 1-30 -> ends 30 Apr)
while I (human) read both of these cases the same
There was a problem hiding this comment.
date resolution is much bigger issue and should be solved in separated issue. please merge this PR
There was a problem hiding this comment.
@aitelint Yes, changes will come as part of another issue. Let me create one soon. We're discussing alternatives as it can be considered a breaking change.
|
@aitelint Can you resolve these conflicts? Thanks. |
conflicts solved |
tellarin
left a comment
tellarin
left a comment
There was a problem hiding this comment.
Approving as-is as to not modify existing behaviour and re-resolution is beyond the scope of the issue this PR fixes.
3 participants
Fix to issue #2850 in NL|EN|FR|DE|IT|PT|ES
Test cases added to DateTimeModels.