Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Translation for DATETIMEFROMPARTS (SqlServer) #18630

Merged
merged 6 commits into from
Dec 17, 2019
Merged

Implement Translation for DATETIMEFROMPARTS (SqlServer) #18630

merged 6 commits into from
Dec 17, 2019

Conversation

nmichels
Copy link
Contributor

smitpatel
smitpatel reviewed Oct 31, 2019
View reviewed changes
Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Approach looks fine. Added few comments.

src/EFCore.SqlServer/Query/Internal/SqlServerDateTimeFromPartsFunctionTranslator.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs Outdated Show resolved Hide resolved
@smitpatel
Copy link
Member

@nmichels - Are you still planning to work on this?

@nmichels
Copy link
Contributor Author

nmichels commented Dec 8, 2019

Sure @smitpatel ! Please let me know what still can be improved.

@smitpatel smitpatel self-assigned this Dec 8, 2019
@smitpatel smitpatel changed the title [WIP] Implement Translation for DATETIMEFROMPARTS (SqlServer) Implement Translation for DATETIMEFROMPARTS (SqlServer) Dec 8, 2019
@nmichels
Implement Translation for DATETIMEFROMPARTS (SqlServer)
bb5713e 
- Fixes dotnet#15283
@nmichels
Refactor:
241926b 
- Removed unecessary type conversions.
- Coding style refactoring.
@nmichels nmichels force-pushed the ImplementCreateDateTimeSqlServer branch 2 times, most recently from 26701f7 to 524cecd Compare December 10, 2019 03:40
@nmichels nmichels force-pushed the ImplementCreateDateTimeSqlServer branch from 524cecd to a77d3d5 Compare December 10, 2019 03:42
@nmichels nmichels force-pushed the ImplementCreateDateTimeSqlServer branch from 62eb15f to 31495a7 Compare December 12, 2019 02:32
smitpatel
smitpatel approved these changes Dec 13, 2019
View reviewed changes
Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Few nits left, most of the implementation is accurate.

@nmichels
Copy link
Contributor Author

Thanks for your help @smitpatel
I will submit the requested changes as soon as possible.

@nmichels
Review Requested Changes
5d41d48 
- Do not allow execution on client
- Add test case using local variable
- Fix docs with proper parameters and DateTime links
@smitpatel smitpatel merged commit 46cfda3 into dotnet:master Dec 17, 2019
@ajcvickers ajcvickers mentioned this pull request Jan 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smitpatel smitpatel smitpatel approved these changes

Assignees

@smitpatel smitpatel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DBFunctions.CreateDateTime
2 participants
@nmichels @smitpatel