-
Notifications
You must be signed in to change notification settings - Fork 15
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 group and type choice parsing and validation errors #121
Fix group and type choice parsing and validation errors #121
Conversation
@tomachristian feel free to give this a sanity check with some of your scenarios. Will get a separate PR up for smoke tests. |
Hello @anweiss. Thank you for this fix, it works to fix specifically that scenario, but I found one where it does not work
with CBOR
it fails with
Thank you! |
Thanks @tomachristian! Keep 'em coming! I believe I've fixed this with the most recent commit, but feel free to give it a whirl. |
c8aa8ad
to
9a27fc6
Compare
Scratch that ... need to fix more of the logic here. |
@tomachristian I think I've got this fixed now |
Thank you, @anweiss! This specific scenario works, but found this:
works with input
but, this
does NOT work with the same input. Also, it seems that validation of float ranges requires the input to be a float, it does not work with ints event though ints are floats.
is ok with PS: Sorry I placed everything in here, we can split them in separate issues if you want, I don't mind. Thank you, again! |
Thanks @tomachristian. Working through some more fixes. |
hey @tomachristian, pushed up a couple of tweaks. Not 100% sure if I've fixed the issue, but feel free to throw some more examples at it and let me know if something still isn't working. Also re. range validation, I interpreted the spec quite literally ... per https://datatracker.ietf.org/doc/html/rfc8610#section-2.2.2.1:
Thus, if it is a floating point range, validation should only succeed against a float value that has not been coerced from an integer. CC @cabo for thoughts. Happy to move this to a Discussion as needed. |
Fixes #116 and #120.