Skip to content

Proposed fix for not null enum type#314

Merged
doron050 merged 5 commits intoDaredevilOSS:mainfrom
ballistic-booger:patch-1
Sep 12, 2025
Merged

Proposed fix for not null enum type#314
doron050 merged 5 commits intoDaredevilOSS:mainfrom
ballistic-booger:patch-1

Conversation

@ballistic-booger
Copy link
Contributor

This drops the .Value call on non nullable enum types, which doesn't exist

This drops the `.Value` call on non nullable enum types, which doesn't exist
@ballistic-booger
Copy link
Contributor Author

@doron050 I fixed up the line ending that caused the linting to fail, would you mind kicking off the workflow again? Cheers

@doron050
Copy link
Collaborator

@doron050 I fixed up the line ending that caused the linting to fail, would you mind kicking off the workflow again? Cheers

Hi, first of all, thanks for your contribution!
I think you should run locally dotnet format using the .NET 8 version and that should fix the issue
Second, do you think that you could add a test that uses a not null enum type to make sure this case is covered and will not break in the future? This should be easy
Take a look at other tests in https://github.com/DaredevilOSS/sqlc-gen-csharp/blob/main/end2end/EndToEndScaffold/Templates/PostgresTests.cs and run the test generate command in the makefile generate-end2end-tests

If not, it's OK, I'll add the test after the CI passes

- use dotnet 8
@ballistic-booger
Copy link
Contributor Author

@doron050 I fixed up the line ending that caused the linting to fail, would you mind kicking off the workflow again? Cheers

Hi, first of all, thanks for your contribution! I think you should run locally dotnet format using the .NET 8 version and that should fix the issue Second, do you think that you could add a test that uses a not null enum type to make sure this case is covered and will not break in the future? This should be easy Take a look at other tests in https://github.com/DaredevilOSS/sqlc-gen-csharp/blob/main/end2end/EndToEndScaffold/Templates/PostgresTests.cs and run the test generate command in the makefile generate-end2end-tests

If not, it's OK, I'll add the test after the CI passes

I had a quick look, and I'll be honest I didn't look very long / hard so excuse my ignorance if I'm way off.. but the existing tests in end2end/EndToEndScaffold... seem like they're asserting runtime behaviour of the generated code.

In the bug above the generated code doesn't compile while performing .Value on a non-nullable enum:
image

I then had a quick look at CodegenTests which seemed better suited, but there it seemed like it was testing the outcome of generated members.

Again I could be off here, but to me it seems like a unit test is needed asserting the result of the Delegate returned by GetWriterFn at sqlc-gen-csharp/Drivers/NpgsqlDriver.cs:600, matches the expected string outputs.

If it's okay with you I'll leave this one to you.

@doron050
Copy link
Collaborator

@doron050 I fixed up the line ending that caused the linting to fail, would you mind kicking off the workflow again? Cheers

Hi, first of all, thanks for your contribution! I think you should run locally dotnet format using the .NET 8 version and that should fix the issue Second, do you think that you could add a test that uses a not null enum type to make sure this case is covered and will not break in the future? This should be easy Take a look at other tests in https://github.com/DaredevilOSS/sqlc-gen-csharp/blob/main/end2end/EndToEndScaffold/Templates/PostgresTests.cs and run the test generate command in the makefile generate-end2end-tests

If not, it's OK, I'll add the test after the CI passes

I had a quick look, and I'll be honest I didn't look very long / hard so excuse my ignorance if I'm way off.. but the existing tests in end2end/EndToEndScaffold... seem like they're asserting runtime behaviour of the generated code.

In the bug above the generated code doesn't compile while performing .Value on a non-nullable enum:
image

I then had a quick look at CodegenTests which seemed better suited, but there it seemed like it was testing the outcome of generated members.

Again I could be off here, but to me it seems like a unit test is needed asserting the result of the Delegate returned by GetWriterFn at sqlc-gen-csharp/Drivers/NpgsqlDriver.cs:600, matches the expected string outputs.

If it's okay with you I'll leave this one to you.

Hi, of course! I'll add the test and merge the PR
Thanks again for finding the bug and submitting a fix!

@doron050 doron050 merged commit d2f7883 into DaredevilOSS:main Sep 12, 2025
6 checks passed
@ballistic-booger ballistic-booger deleted the patch-1 branch September 13, 2025 00:22
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