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

Add support of Interpolated strings. #530

Merged
merged 3 commits into from
Jul 17, 2021
Merged

Add support of Interpolated strings. #530

merged 3 commits into from
Jul 17, 2021

Conversation

yangzhongke
Copy link
Contributor

Add support of Interpolated strings.
see #527

I have added two test cases: QueryableTests.FormattableString.cs and EntitiesTests.FormattableString.cs. All the tests are passed:
image

@StefH StefH changed the title Add support of Interpolated strings. https://github.com/zzzprojects/S… Add support of Interpolated strings. Jul 1, 2021
@StefH
Copy link
Collaborator

StefH commented Jul 1, 2021

Hello @yangzhongke, thank you for your contribution.

However, I thought that this functionality would be integrated in the current code, and not creating new extension methods.

Can you change your PR?

@yangzhongke
Copy link
Contributor Author

yangzhongke commented Jul 2, 2021

If there are two overloaded methods as below:

  1. Where(string predicate,params object[] args)
  2. Where(FormattableString predicate)

Due to the deliberate decision by the Microsoft , The code Where($"Id={id}") will invoke Where(string predicate,params object[] args) instead of Where(FormattableString predicate), which lead to my decision to create new method names, like WhereInterpolated instead of using Where.

@StefH
Copy link
Collaborator

StefH commented Jul 2, 2021

Can't you just add a new method like?

Existing...

public static IQueryable Where([NotNull] this IQueryable source, [NotNull] ParsingConfig config, [NotNull] string predicate, params object[] args)

New...

public static IQueryable Where([NotNull] this IQueryable source, [NotNull] ParsingConfig config, [NotNull] FormattableString predicate)

@yangzhongke
Copy link
Contributor Author

yangzhongke commented Jul 3, 2021

I can. But where($" id={id}") will invoke the "string predicate" version instead of "FormattableString" version.
So I have to create the WhereInteroplated(FormattableString fs) method to match WhereInteroplated($" id={id}").
I have mentioned the reason before.
Make sense?

@yangzhongke
Copy link
Contributor Author

Even EF Core created two methods to execute sql statements, ExecuteSqlRaw(string version) and ExecuteSqlInterpolated(FormatableString version). So I reckon that the two different methods solution, Where and WhereInterpolated , is a tradeoff.

@yangzhongke
Copy link
Contributor Author

Agree with the renaming.
Should I do something to do the changes?

@StefH
Copy link
Collaborator

StefH commented Jul 8, 2021

@yangzhongke
Yes, if possible, change your PR code.

…rmattableStringExtensions.cs and DynamicQueryable_FormattableString_Extensions.cs --> DynamicQueryableWithFormattableStringExtensions.cs
@yangzhongke
Copy link
Contributor Author

I have renamed those two files and committed the changes.
There are two files: DynamicQueryableWithFormattableStringExtensionsand EFDynamicQueryableWithFormattableStringExtensions.
DynamicQueryableWithFormattableStringExtensions.cs contains: AllInterpolated and other XXXInterpolated methods, and EFDynamicQueryableWithFormattableStringExtensions contains: AllInterpolatedAsync and other XXXInterpolatedAsync methods.
DynamicQueryableWithFormattableStringExtensionsand .cs is already on the src/System.Linq.Dynamic.Core folder; because EFDynamicQueryableWithFormattableStringExtensions uses Microsoft.EntityFrameworkCore, just like EntityFrameworkDynamicQueryableExtensions.cs, so EFDynamicQueryableWithFormattableStringExtensions should be on the folder Microsoft.EntityFrameworkCore.DynamicLinq.EFCore3.

@StefH StefH merged commit 4008215 into zzzprojects:master Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants