Skip to content

Conversation

@fdegiuli
Copy link

@fdegiuli fdegiuli commented Feb 11, 2022

Enables IN/NIN whereHelper function generation for nullable types addressing issue #851. Based on #910.

The generated helpers take the underlying primitive types, so that a null.String column would generate a function with signature IN(slice []string) qm.QueryMod

I implemented it this way (instead of accepting nullable values) for a few reasons:

  1. The code is much simpler
  2. I imagine most people aren't using null.* to construct these clauses currently
  3. A NULL in an IN clause is meaningless in SQL anyway, as it does not affect the result

@fdegiuli
Copy link
Author

fdegiuli commented Apr 29, 2022

@stephenafamo do you need anything from me in order to merge this PR?
As far as I can tell there are no existing tests for this part of the code generation, but I did generate the test schema and made sure that everything worked as expected.
It doesn't seem worth testing the text helpers since the test for a function that simple would look exactly like the original function anyway, but I'm happy to add some if you want.

@stephenafamo
Copy link
Collaborator

I haven't had time to manually check it. There was also some discussion about using a potential null/v9 package which would be a breaking change in it self because of the conversation about null/value/set.

Basically, a mix between testing it and making sure there are no breaking changes.

@razor-1
Copy link
Contributor

razor-1 commented Jul 9, 2022

+1 for this, would be really great to have

@Rosswell
Copy link

Rosswell commented Aug 10, 2022

I haven't had time to manually check it. There was also some discussion about using a potential null/v9 package which would be a breaking change in it self because of the conversation about null/value/set.
Basically, a mix between testing it and making sure there are no breaking changes.

@stephenafamo is there a way we can help you test it if you're short on time?

@stephenafamo
Copy link
Collaborator

Really anyone just using the branch and confirming that it works and doesn't introduce any breaking changes.

@razor-1
Copy link
Contributor

razor-1 commented Aug 10, 2022

I have included this into my fork and it works fine for my use case.

@stephenafamo
Copy link
Collaborator

@fdegiuli kindly fix the merge conflicts so I can merge this

@fdegiuli
Copy link
Author

fdegiuli commented Aug 16, 2022 via email

@stephenafamo stephenafamo merged commit 78272e8 into aarondl:master Aug 19, 2022
@lallenfrancisl
Copy link

@stephenafamo any hope if this can be pushed in a v4 release, kind of a blocker in a project ?

@stephenafamo
Copy link
Collaborator

I'll tag a release within the next week.

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.

5 participants