Add a separate resolver for records #4971
Conversation
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
One thought... Ideally, if some attempts to read a record but they haven't opted into records, they'd get an informative exception telling them how to enable support (rather than some "unknown/unhandled type" message). This should be possible by having a dummy handler in the built in resolver, right?
vonzshik
force-pushed
the
4965-record-separate-resolver
branch
from
4410b90 to
078bba6
Compare
Done. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Looks good. Consider adding a test for the case when records having been enabled (both to ensure they're not enabled by default and to check the exceptione tc.)
Sure. Just FYI with this change we lose about 10kb (we are still in 21kb positive, but yeah) |
|
Just for adding UnsupportedTypeHandler? Seems like a lot :/ But I still think it's important to have a decent experience when using NpgsqlSlimDataSourceBuilder... probably worth it? |
Yep, just for adding. Agree that it seems quite big, though it should be one-time only, so I would say it's still fine to do this. |
|
Added a test and moved a few things around. |
| /// </summary> | ||
| class MiscTypeTests : MultiplexingTestBase | ||
| { | ||
| NpgsqlDataSource _slimWithoutMappingsDataSource = null!; |
There was a problem hiding this comment.
Not sure there's any point to have these on the class - can just have two tests which each manages its own builder inside the tests.
See e.g. what I did for ranges.
There was a problem hiding this comment.
Not sure there's any point to have these on the class - can just have two tests which each manages its own builder inside the tests.
The point was that we're going to split BuiltInTypeResolver in quite a few resolvers. so we can reuse the exact same data source between the tests for each type.
There was a problem hiding this comment.
As you wish - though I'm not sure it's worth doing that kind of stuff (with class setup/teardown)... Can just as well have every test completely self-contained, adding (or not) exactly what they want on the slim builder
There was a problem hiding this comment.
As you wish - though I'm not sure it's worth doing that kind of stuff (with class setup/teardown)... Can just as well have every test completely self-contained, adding (or not) exactly what they want on the slim builder
Just for you, I've removed them 😉
3 participants
Part of #4965
This change saves us around 23kb on windows and linux.