-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate to date and time utils from frontend-shared #6587
Conversation
3ee29a6
to
7b08130
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6587 +/- ##
==========================================
- Coverage 99.43% 99.42% -0.01%
==========================================
Files 270 270
Lines 10240 10169 -71
Branches 2425 2417 -8
==========================================
- Hits 10182 10111 -71
Misses 58 58 ☔ View full report in Codecov by Sentry. |
31fefe7
to
ac2448b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reviewing this I realized that we've lost the context around why we use this particular date time format for text exports. I think the rationale was to have a human friendly version of the ISO date time format. Can you remember any other reasons? Lexicographic sortability isn't an obviously critical thing in this context.
@@ -91,7 +91,7 @@ export class AnnotationsExporter { | |||
const page = pageLabel(annotation); | |||
const annotationQuote = quote(annotation); | |||
const lines = [ | |||
`Created at: ${formatDateTime(new Date(annotation.created))}`, | |||
`Created at: ${formatSortableDateTime(new Date(annotation.created))}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me why we needed to use a date/time format that has this lexicograph / chronological sorting equivalence?
#6079 was the PR that introduced the change, but the only rationale in that PR was to have a more human-friendly form of the ISO date.
Yeah... This is unfortunate, but I also don't fully remember why we choose this particular format. I can only guess it was so that in the case of, for example, CSV, resulting datasets could be sorted by that column, but I'm only speculating here. I'll try to dig a bit further into old discussions and PRs. |
I've been digging a bit, and I think we went with this format because it is the format used in the examples originally provided by the product team here https://docs.google.com/document/d/1Xn6AazKkpUy85VUaj9v_utasexk8lIqP_1zveILq1So/edit The format there includes seconds as well, but I remember discussing we didn't need that much precision and we left it at |
Oh, I found this other PR where this function was implemented #6078 It mentions not only the fact that those dates can be sorted, but also that they are detected as "date" by most common spreadsheet editors. I'll add that to the function comment as well. |
2d16ab9
to
4542c6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some small requested changes.
src/sidebar/components/Annotation/test/AnnotationTimestamps-test.js
Outdated
Show resolved
Hide resolved
4542c6a
to
3f3c9cd
Compare
Depends on hypothesis/frontend-shared#1726
Use date utils from frontend-shared instead of local implementations.
Additionally, a remaining standalone function called
formatDateTime
has been renamed toformatSortableDateTime
, to differentiate from theformatDateTime
now provided by frontend-shared.The reason to choose this name is that the format returned by this function is
YYYY-MM-DD hh:mm
, which produces values that are sorted chronologically when sorted alphabetically.TODO
formatDateTime
function, to avoid confusions with frontend-shared one, which returns a different format.