-
Notifications
You must be signed in to change notification settings - Fork 225
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 for code generation for List<X> types #2402
base: main
Are you sure you want to change the base?
Conversation
I also added a few additional tests to verify that array type columns are being generated properly as well as inserting default and HasData as I could not find any |
This seems like it adds support for |
lol I agree yes that's exactly what I did. Let me fix that. I didn't realize there was a ListInit expresion function. I was having trouble finding it but: there it is :) |
Looks like there's a problem with that. When I use the following code: public override Expression GenerateCodeLiteral(object value)
{
var values = (System.Collections.IList)value;
List<Expression> elements = new(values.Count);
var generated = true;
foreach (var element in values)
{
if (generated)
{
try
{
elements.Add(ElementMapping.GenerateCodeLiteral(element)); // attempt to convert if required
continue;
}
catch (NotSupportedException)
{
generated = false; // if we can't generate one element, we probably can't generate any
}
}
elements.Add(Expression.Constant(element));
}
return Expression.ListInit(Expression.New(typeof(List<>).MakeGenericType(ElementMapping.ClrType)), elements);
} I get the error: The literal expression 'new List`1() {Void Add(Int32)(1), Void Add(Int32)(2), Void Add(Int32)(3)}' for 'List<int>' cannot be parsed. Only simple constructor calls and factory methods are supported. so it looks like we might have to stick with the array initializer calling ToList() on it. |
IIRC this is why I originally implemented array literals but not List... I think we should simply add support for this on the EF side - it shouldn't be too hard. Then we'd be able to generate the right thing here. |
I certainly don't mind doing that either. If you have a hint of where you think it might be it would be exceedingly helpful. |
Fixes dotnet#19274 Also relates to npgsql/efcore.pg#2402
Alrighty. I think what I've done should fix this issue. In fact, it might just fix it without the code in this PR since I've now added support for List literals in EF Core but I'm going to test locally and see what the outcome is. |
Yup.... this PR isn't even necessary once the other PR to EFCore is merged. All my test cases I wrote as part of this PR now pass without the code changes to NgpgsqlArrayTypeMapping or NgpgsqlArrayListTypeMapping if I change my local NuGet to point to the compiled source from the other PR. Would you like to me to just close this PR completely or leave it open with just the tests pending the other PR being merged so that we have additional tests for these cases? |
Fixes dotnet#19274 Also relates to npgsql/efcore.pg#2402
Thanks @yinzara, yeah - having tests here would be a good thing. Once the support is merged EF-side, I'll sync here to use the latest version and we can merge tests. |
Fixes dotnet#19274 Also relates to npgsql/efcore.pg#2402
Fixes dotnet#19274 Also relates to npgsql/efcore.pg#2402
Fixes dotnet#19274 Also relates to npgsql/efcore.pg#2402
* Add support for CSharpHelper for List literals Fixes #19274 Also relates to npgsql/efcore.pg#2402 * Fixes from review comments
@yinzara the main branch of EFCore.PG has been synced to the latest EF Core daily build, which contains your List support from dotnet/efcore#28212. So you can add tests now. |
@yinzara are you planning to add the appropriate tests here? |
Fixes #2401