-
Notifications
You must be signed in to change notification settings - Fork 332
Align error handling behavior #548
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
Conversation
.into(), | ||
), | ||
}, | ||
_ => Ok(ObjectLen::NoneExisting), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make more sense that this is an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in new behavior it is an error (see json_obj_len
). This is to maintain legacy behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why add it as part of the valid return value and not as error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for backward compatibility with V 1.0 (only with legacy path)
JSON.OBJKEYS doc1 .nowhere
and JSON.OBJKEYS no_such_doc .
should return null (as before)
tests/pytest/test.py
Outdated
@@ -745,7 +745,7 @@ def testLenCommands(env): | |||
r.assertEqual(r.execute_command('JSON.ARRLEN', 'test', '.arr'), 6) | |||
|
|||
# test elements with undefined lengths | |||
r.assertEqual(r.execute_command('JSON.ARRLEN', 'test', '.bool'), None) | |||
r.expect('JSON.ARRLEN', 'test', '.bool').raiseError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let add .contains
with a substring we expect to see in the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some comments.
Also why did you updated readies submodule?
Build was failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one followup question on one of the comment, but no a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
.into(), | ||
), | ||
}, | ||
_ => Ok(ObjectLen::NoneExisting), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for backward compatibility with V 1.0 (only with legacy path)
JSON.OBJKEYS doc1 .nowhere
and JSON.OBJKEYS no_such_doc .
should return null (as before)
Maintain legacy behavior
And some tuning of new behavior