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(dynamite): Fix pattern check for enums #1313

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

provokateurin
Copy link
Member

The pattern has to be checked after the value has been serialized (not only applies to enums, but that's the only use case so far).

Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

Please add a unit test to dynamite_e2e.
I'd suggest a new spec just for pattern checking.

For enums we should be able to verify the pattern at generation time as the value of an enum is constant. Should we do so long term? If so we should open an issue for that.

@provokateurin
Copy link
Member Author

Yes, I forgot to add the test before creating the PR 😅
Checking enums at build time sounds good, for other cases we still need this change. Can you open an issue for that?

@provokateurin provokateurin force-pushed the fix/dynamite/pattern-check-enums branch from ed5000c to 1330c36 Compare December 18, 2023 08:34
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

@provokateurin
Copy link
Member Author

Right but in this case I don't want to test the pattern checking itself but that it works with serialized parameters (which was broken before).
Also I made a mistake which I'm fixing right now.

@provokateurin provokateurin force-pushed the fix/dynamite/pattern-check-enums branch from 1330c36 to 7cdd1ec Compare December 18, 2023 08:55
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

blocking for now

@provokateurin provokateurin force-pushed the fix/dynamite/pattern-check-enums branch from 7cdd1ec to 9fbff5a Compare December 18, 2023 11:56
@provokateurin provokateurin force-pushed the fix/dynamite/pattern-check-enums branch from 9fbff5a to 74b1f8a Compare December 18, 2023 12:08
@provokateurin provokateurin force-pushed the fix/dynamite/pattern-check-enums branch from 74b1f8a to befbd0b Compare December 18, 2023 12:23
@provokateurin
Copy link
Member Author

A null input for the pattern checks is always allowed. This was not handled correctly and the tests for it were also wrong. If the input is not allowed to be null then it needs to be checked elsewhere and not with the patterns.

@provokateurin provokateurin merged commit 8974a97 into main Dec 18, 2023
8 checks passed
@provokateurin provokateurin deleted the fix/dynamite/pattern-check-enums branch December 18, 2023 13:28
@Leptopoda Leptopoda removed this from the Dynamite packages release milestone May 3, 2024
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.

2 participants