Skip to content

Conversation

@vthemelis
Copy link

@vthemelis vthemelis commented Oct 6, 2024

Fixes: #7260
Fixes: #7261

Timestamp contains UTC information so it makes sense to capture it in DateTimeOffset rather than DateTime.

Also, capture Date32 as a DateOnly if we're in .NET 6 (DateOnly is missing from .NET standard at this point).


We are excited to review your PR.

So we can do the best job, please check:

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.
@vthemelis
Copy link
Author

@dotnet-policy-service agree

@vthemelis
Copy link
Author

@vthemelis vthemelis force-pushed the add-timestamp-and-date32 branch 4 times, most recently from c370a14 to 198834c Compare October 6, 2024 21:44
Timestamp contains UTC information so it makes sense to capture it in `DateTimeOffset` rather than `DateTime`. Also, capture `Date32` as a `DateOnly` if we're in .NET 6 (`DateOnly` is missing from .NET standard at this point).
@vthemelis vthemelis force-pushed the add-timestamp-and-date32 branch from 198834c to ebc6811 Compare October 6, 2024 22:49
@codecov
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 16 lines in your changes missing coverage. Please review.

Project coverage is 68.85%. Comparing base (1e91427) to head (13ce456).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
...eDataFrameColumns/DateTimeOffsetDataFrameColumn.cs 9.09% 10 Missing ⚠️
...icrosoft.Data.Analysis/PrimitiveDataFrameColumn.cs 75.00% 2 Missing and 3 partials ⚠️
...Microsoft.Data.Analysis.Tests/DataFrame.IOTests.cs 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## main #7262 +/- ## ========================================== + Coverage 68.78% 68.85% +0.06%  ========================================== Files 1463 1474 +11 Lines 272288 274195 +1907 Branches 28177 28423 +246 ========================================== + Hits 187300 188791 +1491  - Misses 77748 78084 +336  - Partials 7240 7320 +80 
Flag Coverage Δ
Debug 68.85% <66.66%> (+0.06%) ⬆️
production 63.30% <55.88%> (+0.01%) ⬆️
test 89.18% <92.85%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Microsoft.Data.Analysis/DataFrame.Arrow.cs 98.60% <100.00%> (+6.99%) ⬆️
...imitiveDataFrameColumns/DateTimeDataFrameColumn.cs 90.90% <ø> (ø)
...est/Microsoft.Data.Analysis.Tests/ArrayComparer.cs 64.40% <ø> (+0.84%) ⬆️
...osoft.Data.Analysis.Tests/ArrowIntegrationTests.cs 100.00% <100.00%> (ø)
...Microsoft.Data.Analysis.Tests/DataFrame.IOTests.cs 99.14% <90.00%> (+0.01%) ⬆️
...icrosoft.Data.Analysis/PrimitiveDataFrameColumn.cs 72.81% <75.00%> (+0.27%) ⬆️
...eDataFrameColumns/DateTimeOffsetDataFrameColumn.cs 9.09% <9.09%> (ø)

... and 57 files with indirect coverage changes

@vthemelis
Copy link
Author

vthemelis commented Oct 7, 2024

I think I'm happy with the coverage report. The lines that matter are covered.

Copy link
Contributor

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

Just some minor changes but otherwise looks good to me.

@vthemelis
Copy link
Author

Just some minor changes but otherwise looks good to me.

Thank you very much for the quick review @michaelgsharp ! I've added a few replies to your comments to help figure out the best course of action before proceeding with this.

@vtgruk
Copy link

vtgruk commented Oct 15, 2024

Hi @michaelgsharp, would you mind having another look at this?

@vthemelis
Copy link
Author

Hi @michaelgsharp , would you be able to have a look at this?

@vthemelis
Copy link
Author

@michaelgsharp could you have another look? I addressed all of your comments so should be straightforward to review.

@vthemelis vthemelis force-pushed the add-timestamp-and-date32 branch from b626e57 to 13ce456 Compare November 10, 2024 12:13
@vthemelis vthemelis closed this Nov 26, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.