Conversation
4678af7 to
8adda02
Compare
8adda02 to
9a993e3
Compare
|
The diff is large, but these are the only substantial changes: + /// <inheritdoc />
protected NpgsqlListTypeMapping(RelationalTypeMappingParameters parameters, RelationalTypeMapping elementMapping)
- : base(parameters) {}
+ : base(parameters)
+ => ElementMapping = elementMapping;- if (arr.Rank != 1)
+ if (list.GetType().GenericTypeArguments[0] != ElementMapping.ClrType)
throw new NotSupportedException("Multidimensional array literals aren't supported");
var sb = new StringBuilder();
sb.Append("ARRAY[");
- for (var i = 0; i < arr.Length; i++)
+ for (var i = 0; i < list.Count; i++) |
| { | ||
| // TODO: Duplicated from NpgsqlArrayTypeMapping | ||
| var arr = (Array)value; | ||
| var list = (IList)value; |
There was a problem hiding this comment.
@austindrenski I'm not too sure about this...
For one thing multidimensional arrays seem to implement List - but not List<T>. This is odd, and I suspect it to be .NET legacy behavior which was kept to maintain backwards compatibility.
At the very least, please add a specific check for multidimensional arrays, throwing NotSupportedException - it's an error to render a multidimensional .NET array as a one-dimensional PostgreSQL array...
(it would have been better to allow reviewing this non-cosmetic change before merging, but no big deal :)).
There was a problem hiding this comment.
Admittedly, I wasn't thinking about multidimensional arrays here...which underscores your point about reviews—that's my bad.
Since we don't support jagged arrays, I assumed that List<T> would only be mappable to one-dimensional PostgreSQL arrays. (Since List<List<T>> is more or less the equivalent of T[][].)
Did you have something else in mind?
There was a problem hiding this comment.
See some additional thoughts in #552 (comment).
As I wrote there, we basically need to separate between multidimensional arrays and jagged arrays. The latter really aren't supported by PostgreSQL and a clear, informative message should say so. I agree that List<List<T> is just another form of a jagged array and should throw in the same way.
There's no such thing as a multidimensional array with List<T>, so I don't think you need to think about that here in NpgsqlListTypeMapping (as opposed to NpgsqlArrayTypeMapping).
Aside from that, I'm simply not sure there's value in support non-generic List - I'm assuming part of you change was to support that. It's probably healthier to restrict support to generic List<T> only.
Hope this is clear, we're discussing several different things here...
There was a problem hiding this comment.
So, I think we're on the same page. But I'm still a little lost on what you mean by the non-generic List:
Aside from that, I'm simply not sure there's value in support non-generic
List- I'm assuming part of you change was to support that.
- My understanding is that we will only support
List<T>whereTis not a collection type. - I cast the incoming parameter to
IListso that I could accessIList.Countfor the loop below.- I'm using
IListsince the type mapper is non-generic, so there's no type parameter in scope forIList<T>/List<T>.
- I'm using
There was a problem hiding this comment.
Yeah, I wasn't very clear on that.
First, I'd rather we had a clear up-front check that GenerateNonNullSqlLiteral() gets the right kind of input, i.e. a generic List<T> where T corresponds to ElementMapping. Right now we throw with imprecise messages (multidimensional arrays, or just that the given object isn't a generic type).
Second, as I wrote in the other comment, I'd rather we had construction-time checks for the correctness of the mapping object itself, i.e. throw if we attempt to construct a mapping that would correspond to a jagged array (i.e. T[][], List<List<T>>).
After all these checks it seems fine to cast to IList as we've verified that the types are correct.
There was a problem hiding this comment.
Got it. I'll put something together in the next few days.
| static List<TElem> Snapshot(List<TElem> source, ValueComparer<TElem> elementComparer) | ||
| { | ||
| if (source == null) | ||
| if (source is null) |
There was a problem hiding this comment.
Is there any specific reason to convert x == null to x is null? It makes sense if you use pattern matching (i.e. x is null y) but just for null checking? I'd stick to the current standard syntax rather than starting to change the entire codebase etc.
There was a problem hiding this comment.
That's a fair point. I tend to make these changes out of habit more than anything else.
I'll revert this in an upcoming commit. (This type mapper still lacks type-casting for the array literals.)
Moved from #541.