Skip to content
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

Revert "Fix to #18147 - Where bool column needs to convert to equalit… #20425

Merged
merged 1 commit into from
Mar 27, 2020

Conversation

smitpatel
Copy link
Member

@smitpatel smitpatel commented Mar 26, 2020

…y when value converter is applied"

This reverts commit e922850.

@smitpatel smitpatel requested a review from maumar March 26, 2020 23:27
@smitpatel
Copy link
Member Author

Will merge on build pass. We can discuss later in #18147

…y when value converter is applied"

This reverts commit e922850.

# Conflicts:
#	test/EFCore.Specification.Tests/CustomConvertersTestBase.cs
#	test/EFCore.SqlServer.FunctionalTests/CustomConvertersSqlServerTest.cs
#	test/EFCore.Sqlite.FunctionalTests/CustomConvertersSqliteTest.cs
@smitpatel smitpatel merged commit 8bd99a9 into master Mar 27, 2020
@smitpatel smitpatel deleted the smit/incorrectfix branch March 27, 2020 00:25
@roji
Copy link
Member

roji commented Mar 28, 2020

Note to @smitpatel @maumar: when syncing EFCore.PG against ac46335 (latest nuget available), I get a test failure on Where_bool_gets_converted_to_equality_when_value_conversion_is_used_using_EFProperty, where PostgreSQL complains that WHERE must receive a boolean type, not character varying:

SELECT b."BlogId", b."Discriminator", b."IndexerVisible", b."IsVisible", b."Url", b."RssUrl"
        FROM "Blog" AS b
        WHERE b."IsVisible"

(same with Where_bool_gets_converted_to_equality_when_value_conversion_is_used_using_indexer)

I see the change has been reverted already, I guess just keep in mind for when fixing again.

@roji
Copy link
Member

roji commented Mar 28, 2020

Oops, was looking at the wrong commit. I'm actually synced against 8771406, which already has this revert inside it - the test still fails. Do you guys want a new issue to track this?

@smitpatel
Copy link
Member Author

#18147 has been re-opened. I left skipped tests behind in this PR. Tests are not skipped in specs since SqlServer can run them. Individual providers have skipped them.

You need some sleep @roji

@roji
Copy link
Member

roji commented Mar 28, 2020

Thanks @smitpatel, good to know. And I definitely do need some sleep at some point.

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.

2 participants