Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Some test simplifications - #4989

#4989
Closed
roji wants to merge 1 commit into
npgsql:mainnpgsql/npgsql:mainfrom
roji:TestStuffroji/Npgsql:TestStuffCopy head branch name to clipboard
Closed

Some test simplifications#4989
roji wants to merge 1 commit into
npgsql:mainnpgsql/npgsql:mainfrom
roji:TestStuffroji/Npgsql:TestStuffCopy head branch name to clipboard

Conversation

@roji

@roji roji commented Mar 10, 2023

Copy link
Copy Markdown
Member

No description provided.

@roji
roji requested a review from vonzshik as a code owner March 10, 2023 13:40
Comment thread test/Npgsql.Tests/Types/RangeTests.cs Outdated
public async Task Ranges_not_supported_with_slim_builder_without_EnableRanges()
{
await using var slimDataSource = new NpgsqlSlimDataSourceBuilder(ConnectionString).Build();
await AssertTypeUnsupportedRead<NpgsqlRange<int>, InvalidCastException>(@"[1,2)", "int4range", slimDataSource);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4971 is going to make it so we throw NotSupportedException if we attempt to read a record without EnableRecords. We probably should align them (return unsupported handler for ranges?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, that would be great. This way the user also gets the informative message when they attempt to read a range.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably in a separate pr. Also, might be a good idea to make a separate resolver with all of the unsupported types (instead of adding them to BuiltInTypeResolver)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for separate PR. Any particular reason for the separate resolver?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It will make BuiltInTypeResolver a bit faster (for example switch over dataTypes might be a bit more optimized since there will be less values)
  2. Just having all of them bundled in a separate file is nice, BuiltInTypeResolver only handles things it supports, UnsupportedTypeResolver only handles things it doesn't support :trollface:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I doubt there will be a noticeable perf improvement, but why not.

@roji
roji force-pushed the TestStuff branch 2 times, most recently from 2db3f5a to 7a28f86 Compare March 10, 2023 15:31
@NinoFloris

Copy link
Copy Markdown
Member

This was already merged in a while ago via #5279

@NinoFloris NinoFloris closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.