-
Notifications
You must be signed in to change notification settings - Fork 877
Make arrays opt-in (superceded) #5125
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
Conversation
| /// <summary> | ||
| /// Sets up mappings for the arrays. | ||
| /// </summary> | ||
| public NpgsqlSlimDataSourceBuilder EnableArrays() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably think about this in the context of my rework, as we really should design for trimability here.
The difficulty lies with the globalness of this method (it knows nothing about individual resolvers) while having to opt-in per resolver to get static trimability of not including all the array mappings.
Maybe we can do something clever with an interface on resolvers that support arrays to be able to pull out the WithArray resolver type (and replace the resolver in the list for the new 'WithArray' version).
At that point - as far as I understand - all implementations of that interface will get rooted, basically global opt-in with extra steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably the same thing applies for ranges etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ranges are a bit different in the sense that they're not an automatically derived type. A range is its own type (and you can have an array of that range again). So you're right in the sense that we probably should rename EnableRanges to EnableBuiltinRanges given those are the ranges we'll start supporting.
Multiranges is probably the best analog to arrays as it's similarly automatically there, just only for range types. Those probably deserve the same cross-cutting opt-in treatment as arrays should ideally have :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, built-in ranges are a very constrained set, whereas there's an array for every base type (and more), that's true.
Closes npgsql#4988 Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
|
@roji do you want me to take a look or we're putting this on break until @NinoFloris's changes are merged? |
|
Definitely not - this is superceded by @NinoFloris's work... However, there's some test work I did in this PR which we should still merge (some I already brought into Nino's PR, but not sure if everything). I'll do that soon. |
Based on @vonzshik's work in #5009. Notes:
Closes #4988