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.

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

Added Convert.ToDateTime as a supported method for translation #1297 #1298

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TanielianVB
Copy link

It could not be so simple as that right? But I could not find the unit tests that asserted the translated query from the other Convert methods so... How could I test it?

Sorry in advance if I took an unnecessary step.

@TanielianVB
Copy link
Author

TanielianVB commented Mar 4, 2020

Maybe it would have been best if I didn't created a pull request so soon. I could have asked the same referencing the commit on my fork without creating a pull request.

@roji
Copy link
Member

roji commented Mar 4, 2020

@TanielianVB no worries about creating a PR!

Yeah, it actually is that simple (though a test would also be good). The reason this hasn't been done is that unlike Convert.ToInt32, Convert.ToDateTime would not work on the database in the same way as it would work in C#: the date format strings accepted by PostgreSQL and by .NET ToDateTime are very different. Let's wait for some feedback in #1297 before doing anything.

@TanielianVB
Copy link
Author

I think that if a dev uses Convert.ToDateTime he has to know in what SQL the query will be translated. But, we can also add functions like EF.Functions.ILike to the TO_DATE and TO_TIMESTAMP functions.

@roji
Copy link
Member

roji commented Mar 4, 2020

The general principle in EF is for methods to behave in the same way when translated to the server, as they would when executed on the client - this is why Convert.ToDateTime is somewhat problematic... But you're right that in any case we can provide something on EF.Functions.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review this.

Can you please add a test to NorthwindFunctionsQueryNpgsqlTest which follows the model of the tests in EF Core?

@TanielianVB TanielianVB requested a review from roji September 4, 2020 16:17
@TanielianVB TanielianVB requested a review from roji September 4, 2020 17:23
@TanielianVB
Copy link
Author

Already made all requested changes.

@roji roji changed the base branch from dev to main September 17, 2020 14:12
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.

2 participants