concord-consortium / codap

CODAP (Common Online Data Analysis Platform)
MIT License
94 stars 38 forks source link

Implement most of date functions, extract createDate/ and convertToDate, add tests #1368

Closed pjanik closed 1 month ago

pjanik commented 1 month ago

https://www.pivotaltracker.com/story/show/187799270

This PR adds most of the date-related formula implementations. I've also extracted createDate (essentially making the date() formula reuse it 100%, in contrast to the V2 pattern) and convertToDate.

One breaking change compared to V2 is that formulas returning the day or month name now always do so in English. V2 would return these in the local language. On one hand, that's useful. On the other hand, it prevents users from using this value in further calculations (e.g., if (...)). However, if necessary, users can always use formulas that return numeric values. So I wonder if I should actually change it or not.

Finally, the only formula not implemented yet is seconds. This one revealed an issue in the current approach, where dates are converted to strings too early, causing the seconds to be lost in the default date formatting. I'll work on that tomorrow. However, I'm not sure if this is going to be a bigger issue. seconds(date(2020, 5, 4, 3, 2, 1)) should be relatively easy to fix. However, seconds(other_attribute_with_formatted_date) might be much harder (or even impossible) with the current approach where attribute values are always strings and formulas store their results in attribute values.

pjanik commented 1 month ago

Right now, I'm leaning towards reverting this change, as we have number-based equivalents of these functions that can be used in other formulas. Variants that return names could be treated as ones useful for the final output.

It's interesting whether anyone actually used these functions in other formula calculations. If that is the case, we could either break or fix these documents with this updated approach. But it's impossible to say.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 98.48485% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.72%. Comparing base (430e488) to head (3a59eef).

Files Patch % Lines
v3/src/utilities/date-utils.ts 96.87% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 187799270-extend-date-parser #1368 +/- ## ================================================================ - Coverage 85.73% 85.72% -0.02% ================================================================ Files 521 521 Lines 25068 25104 +36 Branches 6813 6816 +3 ================================================================ + Hits 21492 21520 +28 - Misses 3304 3311 +7 - Partials 272 273 +1 ``` | [Flag](https://app.codecov.io/gh/concord-consortium/codap/pull/1368/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | Coverage Δ | | |---|---|---| | [cypress](https://app.codecov.io/gh/concord-consortium/codap/pull/1368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `72.76% <6.66%> (-0.15%)` | :arrow_down: | | [jest](https://app.codecov.io/gh/concord-consortium/codap/pull/1368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `53.40% <98.48%> (+0.06%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cypress[bot] commented 1 month ago

1 flaky test on run #3799 ↗︎

0 198 28 0 Flakiness 1

Details:

Merge pull request #1368 from concord-consortium/187799270-date-formulas-2
Project: codap-v3 Commit: 9f88e7dafc
Status: Passed Duration: 11:02 💡
Started: Jul 23, 2024 12:54 PM Ended: Jul 23, 2024 1:06 PM
Flakiness  cypress/e2e/graph.spec.ts • 1 flaky test View Output
Test Artifacts
Graph UI > graph inspector panel > change points in table and check for autoscale Test Replay Screenshots

Review all test suite changes for PR #1368 ↗︎

bfinzer commented 1 month ago

Regarding monthName() and dayOfWeekName: These are most often used to create a categorical attribute that can be displayed on a graph or map to help a user understand the data. Thus local language is most helpful for this purpose. Using the result in a formula would be rare, and, as you note, better done with numeric values. So I vote for keeping V2 behavior.

pjanik commented 1 month ago

I just pushed a commit that reverts my change and restores V2 behavior. So, I'm merging this PR.