Skip to content

Improve the timestamp parsing#523

Merged
einarmo merged 2 commits intomasterfrom
timestamp-wrapper
Mar 18, 2025
Merged

Improve the timestamp parsing#523
einarmo merged 2 commits intomasterfrom
timestamp-wrapper

Conversation

@einarmo
Copy link
Copy Markdown
Contributor

@einarmo einarmo commented Mar 6, 2025

Add a timestampwrapper config utility, which should make this type easier to use in a good way in config.

Make it accept RFC3339-like datetimes, but also shortened. We require a timezone, which IMO is a good idea, to avoid ambiguity.

@einarmo einarmo requested a review from a team as a code owner March 6, 2025 12:41
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.21%. Comparing base (8b44bc4) to head (84f8e64).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
Cognite.Config/TimestampWrapper.cs 69.56% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   78.10%   78.21%   +0.10%     
==========================================
  Files         123      125       +2     
  Lines       11541    11790     +249     
  Branches     1748     1785      +37     
==========================================
+ Hits         9014     9221     +207     
- Misses       1817     1833      +16     
- Partials      710      736      +26     
Files with missing lines Coverage Δ
Cognite.Common/CogniteTime.cs 83.73% <100.00%> (+3.73%) ⬆️
Cognite.Config/YamlConfigBuilder.cs 81.13% <100.00%> (+0.11%) ⬆️
Cognite.Config/TimestampWrapper.cs 69.56% <69.56%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@einarmo einarmo force-pushed the timestamp-wrapper branch 3 times, most recently from b1e9c8a to 25f17ea Compare March 6, 2025 13:02
ozangoktan
ozangoktan previously approved these changes Mar 6, 2025
Copy link
Copy Markdown
Contributor

@ozangoktan ozangoktan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

ozangoktan
ozangoktan previously approved these changes Mar 6, 2025
@einarmo einarmo requested review from a team and finnag and removed request for a team March 6, 2025 13:19
finnag
finnag previously requested changes Mar 6, 2025
Copy link
Copy Markdown

@finnag finnag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a little more thorough testing

[InlineData("2020-01-01T01:00+01:00", false)]
[InlineData("2020-01-01T01+01:00", false)]
[InlineData("2020-01-01+00:00", false)]
[InlineData("2020-01-01Z", false)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an extremely simplified list of tests?

In a good old test driven methology the winning implementation would be return DateTime(2020, 1, 1, 0, 0, 0, DateTimeKind.Utc) to satisfy all tests.

We should at least test that it parses the numbers in the right order (day/month/year/hour/minute/), that time zones work as intended, that bad dates do NOT parse, and so on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some more test cases, but there really is only so much value in testing a standard library function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@finnag I've updated this, it's ready for another risk review.

Add a timestampwrapper config utility, which should make this type
easier to use in a good way in config.

Make it accept RFC3339-like datetimes, but also shortened. We require a
timezone, which IMO is a good idea, to avoid ambiguity.
@einarmo einarmo requested a review from finnag March 7, 2025 08:08
@einarmo einarmo dismissed finnag’s stale review March 18, 2025 08:36

Try fix list

Copy link
Copy Markdown

@finnag finnag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦄7

@einarmo einarmo merged commit b7d7390 into master Mar 18, 2025
6 checks passed
@einarmo einarmo deleted the timestamp-wrapper branch March 18, 2025 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants