Conversation
|
@slifty Do you think you could try and run this locally? I had some issues with installing the package locally as well and it seems the build fails on the PR too. Do you think you could have a look? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #963 +/- ##
==========================================
+ Coverage 48.79% 49.15% +0.35%
==========================================
Files 351 352 +1
Lines 11345 11439 +94
Branches 1899 1932 +33
==========================================
+ Hits 5536 5623 +87
- Misses 5619 5625 +6
- Partials 190 191 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@aasandei-vsp I was able to run I'm on node 24.13.0 and npm 11.11.0 I haven't yet rebuilt docker, but wanted to give context to the "fix package lock" commit above which appears to at least fix CI. |
|
Looks like the local issue was related to esm / cjs -- apparently the solution is to instruct vite to ignore it during the prebundle step (I admit that even still I'm not fully adept at the nuances of esm) I put these as commits so you see 'em but really they should just be fixup'd into your original introduction of the lib |
aasandei-vsp
force-pushed
the
PER-10481-edtf-service
branch
from
a4b02b0 to
4ee2ce7
Compare
… structured date/time models Introduces EdtfService that handles parsing EDTF (Extended Date Time Format) strings into structured DateTimeModel objects and converting them back. Supports partial years, unspecified fields (XX), date qualifiers (approximate, uncertain), date ranges/intervals, 12-hour AM/PM time conversion, and timezone extraction. Includes validation methods for dates and times, along with comprehensive unit tests and type declarations for the edtf npm package. Issue: PER-10481
aasandei-vsp
force-pushed
the
PER-10481-edtf-service
branch
from
4ee2ce7 to
0e14719
Compare
cecilia-donnelly
left a comment
cecilia-donnelly
left a comment
There was a problem hiding this comment.
This is so fancy, @aasandei-vsp ! I love it. Other than the unknown bit I flagged this looks correct. I can imagine finding other edge cases as we test, but you've organized this really clearly. Thank you!
| const result = service.toDateTimeModel('1985-XX'); | ||
|
|
||
| expect(result.qualifiers.unknown).toBe(true); | ||
| }); |
There was a problem hiding this comment.
I don't think we want unknown to be true here - I believe unknown is a fully null date.
There was a problem hiding this comment.
Getting this from "the extended interval syntax keywords 'unknown' and 'open' have been replaced with null and the double-dot notation ['..'] respectively; " on the spec.
| } | ||
|
|
||
| return `${year}-${date.month}-${date.day}`; | ||
| } |
There was a problem hiding this comment.
I would expect this to return '2026-XX-XX' for something that happened in this year, not just '2026' -- am I misunderstanding? It looks like either is valid, though! "The character 'X' may be used in place of one or more rightmost digits to indicate that the value of that digit is unspecified" -- may so either works. Okay, no action needed!
3 participants
This service is not exposed in any way yet, so it can't be tested. It is only validated atm by unit tests.
The only thing that might cause problems at build time would be the edtf package.
Issue: PER-10481