Enum should allow the conventionally case-sensitive operators #434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jasonkarns I just hopped into the ring to get this project back up to speed, and what you said makes sense to me. Would you mind adding some tests for it? |
|
Related to #403 |
By default (ie, for strings), `eq` is a case-insensitive match and `eql` is case-sensitive. `string_enum` is documented as being "like string but only with case-sensitive `eq`/`not_eq` operators". These two statements feel like contradictions to me. If `string_enum` is basically like `string` but always does case-sensitive filtering, then shouldn't it support the existing case-sensitive operator? Some more rationale why I think this should be the case: polymorphic associations have a _discrete list_ of possible values for the as_type attribute. So to me it's only natural that the as_type attribute be defined as a `string_enum`. (strict case matching!) However, using the polymorphic_has_* helpers results in an relationship filter using the `eql` (case-sensitive) operator. So we have an attribute type that expects to be matched strictly; and a relationship filter that matches strictly. But the two are incompatible! (because string_enum only defines `eq` operator, and polymorphic_has_* helpers define `eql` operator; despite both ostensibly wishing to compare strictly case-sensitve.) So this PR proposes that the enum types _also_ define `eql` operators (which are already expected to be case-sensitive for strings) essentially as duplicates of the existing `eq` operators. Doing so will allow polymorphic as_type attributes to be defined as string_enum and still leverage the `eql` filter applied by the polymorphic_has_* helper. **Warning** no tests yet. This PR primarily for discussion.
github-actions bot
pushed a commit
that referenced
this pull request
Mar 18, 2024
## [1.5.2](v1.5.1...v1.5.2) (2024-03-18) ### Bug Fixes * Enum should allow the conventionally case-sensitive operators ([#434](#434)) ([56d34fd](56d34fd))
|
🎉 This PR is included in version 1.5.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
jkeen
added a commit
to jkeen/graphiti
that referenced
this pull request
Mar 27, 2024
…raphiti-api#434) Add eql and not_eql operators to enum for case-sensitive matching, aligning with string behavior and improving polymorphic association ergonomics --------- Co-authored-by: Jeff Keen <jeff@keen.me>
jkeen
pushed a commit
to jkeen/graphiti
that referenced
this pull request
Mar 27, 2024
## [1.5.2](graphiti-api/graphiti@v1.5.1...v1.5.2) (2024-03-18) ### Bug Fixes * Enum should allow the conventionally case-sensitive operators ([graphiti-api#434](graphiti-api#434)) ([56d34fd](graphiti-api@56d34fd))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By default (ie, for strings),
eqis a case-insensitive match andeqlis case-sensitive.string_enumis documented as being "like string but only with case-sensitiveeq/not_eqoperators".These two statements feel like contradictions to me. If
string_enumis basically likestringbut always does case-sensitive filtering, then shouldn't it support the existing case-sensitive operator?Some more rationale why I think this should be the case:
polymorphic associations have a discrete list of possible values for the as_type attribute. So to me it's only natural that the as_type attribute be defined as a
string_enum. (strict case matching!)However, using the polymorphic_has_* helpers results in an relationship filter using the
eql(case-sensitive) operator. So we have an attribute type that expects to be matched strictly; and a relationship filter that matches strictly. But the two are incompatible! (because string_enum only defineseqoperator, and polymorphic_has_* helpers defineeqloperator; despite both ostensibly wishing to compare strictly case-sensitve.)So this PR proposes that the enum types also define
eqloperators (which are already expected to be case-sensitive for strings) essentially as duplicates of the existingeqoperators. Doing so will allow polymorphic as_type attributes to be defined as string_enum and still leverage theeqlfilter applied by the polymorphic_has_* helper.Warning no tests yet. This PR primarily for discussion.