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

Add Enum support in keyset #54

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Add Enum support in keyset #54

merged 3 commits into from
Feb 2, 2024

Conversation

vpachta
Copy link
Contributor

@vpachta vpachta commented Feb 1, 2024

  • add expression for converting enum to it's underlying type

Closes #29

- add expression for converting enum to it's underlying type
@vpachta
Copy link
Contributor Author

vpachta commented Feb 1, 2024

Implementation for issue #29
Tested with Postgress + Sqlite DB.

Copy link
Owner

@mrahhal mrahhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall this looks good, but I have requested mostly a few minor changes.

Other than that, there's one concern I have regarding the equality part of the expression generation. Usually, we either build a comparison (enum conversion being applied here) or an equality (not being applied). I would've assumed some tests would fail because we're not applying the same enum conversion logic on the equality part (which should be the case in the test case you added), but it seems to be passing. So not sure if this is working properly in some of these cases.

@vpachta
Copy link
Contributor Author

vpachta commented Feb 2, 2024

Thanks! Overall this looks good, but I have requested mostly a few minor changes.

Other than that, there's one concern I have regarding the equality part of the expression generation. Usually, we either build a comparison (enum conversion being applied here) or an equality (not being applied). I would've assumed some tests would fail because we're not applying the same enum conversion logic on the equality part (which should be the case in the test case you added), but it seems to be passing. So not sure if this is working properly in some of these cases.

Actually equal expression works fine for Enums it does not require to convert it to underlying type. The problem was only for comparision and that is why I did not put it for equality part.

@mrahhal mrahhal merged commit 4e8c782 into mrahhal:main Feb 2, 2024
2 checks passed
@mrahhal
Copy link
Owner

mrahhal commented Feb 2, 2024

Thanks!

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.

Add Enum support in keyset
2 participants