Conversation
Summary of ChangesHello @sdahps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Entity Framework Core migrations failed when attempting to map .NET Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix to allow mapping the .NET DateTime type to the Spanner DATE type. The changes include a new SpannerDateDateTimeTypeMapping class, modifications to SpannerDateTypeMapping to support different CLR types, and updated logic in SpannerTypeMappingSource to select the correct mapping. The approach is sound and addresses the issue. I have one suggestion to improve a comment for clarity.
| if (_storeTypeMappings.TryGetValue(storeTypeName, out var mapping) | ||
| || _storeTypeMappings.TryGetValue(storeTypeNameBase, out mapping)) | ||
| { | ||
| // if DateTime is being used to map to DATE we need to use the default DateTime<>DateTime mapping instead of trying to use DateTime<>SpannerDate |
There was a problem hiding this comment.
This comment is a bit misleading. The mapping being returned is for DateTime to the store type DATE, not a DateTime<>DateTime mapping. This logic is special handling for when a DateTime property is explicitly configured to map to a DATE column. A clearer comment would help future maintainers understand this specific logic.
// When mapping DateTime to DATE, use a specific mapping for DateTime. The default DATE mapping is for SpannerDate and uses a converter.
|
Hi @sdahps Sorry for the slow response. Thanks for opening this PR. Would you mind giving a bit of background on exactly what it is that you would want to achieve with this? (And/or add some tests that show how this would be used?) |
2 participants
Fixes #632
Adds a mapping that works for DateTime and DATE, allowing migrations to be created again.
Note: this might be a naive approach, it's a large guess on my part but thought worth showing as the tests still pass.