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

Fix Nullable<bool>.ToString() conversion #33940

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Jun 9, 2024

This PR updates translation of bool? expressions in 2 ways:

  • null is now converted into the empty string, matching the usual C# behavior
  • expressions are assumed to be nullable (they were previously assumed to be non-nullable)

Fixes #33941 for SQLite.
Contributes to #33942.

@ranma42 ranma42 marked this pull request as draft June 9, 2024 19:40
@ranma42 ranma42 force-pushed the fix-nullable-bool-tostring branch 2 times, most recently from e29e284 to 65d134e Compare June 16, 2024 09:35
@ranma42 ranma42 marked this pull request as ready for review June 16, 2024 12:45
@maumar
Copy link
Contributor

maumar commented Jun 18, 2024

There are some product changes here. Consider splitting them out to make this pr test only, alternatively pull in the product changes for #33938 into one pr and update the PR description. Changes look good though.

@ranma42 ranma42 force-pushed the fix-nullable-bool-tostring branch from 65d134e to e766b56 Compare June 18, 2024 12:11
@ranma42 ranma42 marked this pull request as draft June 18, 2024 12:11
@ranma42
Copy link
Contributor Author

ranma42 commented Jun 18, 2024

@maumar I moved the test updates and dataset changes in

I am not 100% sure if this is the "product changes"/all of them; I did not include the tests that were being added as I thought that they might belong to the specific fix/changeset.
I rebased this PR on top of that and marked it as draft as it is basically dependent on it.

Note that in order to ensure that the test suite passes, I disabled ToString_boolean_property_nullable in the PR the base PR and re-enabled it here.

EDIT: I also updated the title and first message of the PR to make it clearer that this one takes care of the fixing, while the other one only improves the tests.

@ranma42 ranma42 changed the title Add test for null value in bool? column and fix ToString conversion Fix Nullable<bool>.ToString() conversion Jun 18, 2024
@ranma42 ranma42 force-pushed the fix-nullable-bool-tostring branch from e766b56 to 00bc998 Compare June 18, 2024 17:24
@ranma42 ranma42 marked this pull request as ready for review June 18, 2024 18:45
@maumar maumar force-pushed the fix-nullable-bool-tostring branch from 00bc998 to 3ff9b77 Compare July 16, 2024 00:38
@maumar
Copy link
Contributor

maumar commented Jul 16, 2024

@ranma42 huge apologies! I wanted to resolve conflicts in #34014 before approval and merge, and I accidentally force pushed into this branch instead (auto-complete >.<) - please tell me you still have these changes locally on your machine...

@ranma42 ranma42 force-pushed the fix-nullable-bool-tostring branch from 3ff9b77 to a2f48ee Compare July 16, 2024 06:22
@ranma42
Copy link
Contributor Author

ranma42 commented Jul 16, 2024

@ranma42 huge apologies! I wanted to resolve conflicts in #34014 before approval and merge, and I accidentally force pushed into this branch instead (auto-complete >.<) - please tell me you still have these changes locally on your machine...

yup :) rebased and pushed this one 👍

also, in case it happens with your own commits (and/or with branches you have not replicated elsewhere, remember that git defaults to keeping commits around for 90 days; git reflog is very effective in saving you from this kind of mistakes

@maumar maumar merged commit ef5693f into dotnet:main Jul 16, 2024
7 checks passed
@maumar
Copy link
Contributor

maumar commented Jul 16, 2024

phew! Thanks making the PR (twice! ;) ) @ranma42

@ranma42 ranma42 deleted the fix-nullable-bool-tostring branch July 21, 2024 10:12
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.

Inconsistent results for Nullable<bool>.ToString()
2 participants