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

Do not evaluate untaken if and switch branches #714

Merged

Conversation

mi25iw
Copy link
Contributor

@mi25iw mi25iw commented Apr 19, 2024

Resolves #713

The samples for json-e indicate that one can check for an value in an if statement and attempt to use it in the 'then' block.

This change delays evaluating the 'then' and 'else' blocks until they are needed, and a similar change is applied to the 'switch' statement.

The change for IfThenElseOperator is fairly straightforward. The SwitchOperator one was as well, although it does change the order from:

  1. Evaluate parameter dictionary (JsonE.Evaluate(parameter, context)!.AsObject();)
  2. Check truthiness
  3. Copy value

to:

  1. Iterate over parameter dictionary (keys have not yet went to JsonE.Evaluate! Not sure if this is important. I saw one code path that looked like it might unescape keys or impact it another way. Please review carefully.)
  2. Check truthiness
  3. Evaluate value (and still copy it since the original code did, I'm new to the codebase so that might not be necessary).

It passed all the tests still, but I am not super comfortable that I changed the order above. I'm hoping you know (or have any easy way to check) if that's problematic.

The samples for json-e indicate that one can check for an value in an
if statement and attempt to use it in the 'then' block.

This change delays evaluating the 'then' and 'else' blocks until they
are needed, and a similar change is applied to the 'switch' statement.
@gregsdennis gregsdennis changed the base branch from master to jsone/update April 19, 2024 01:26
@gregsdennis
Copy link
Owner

Nice! I'll pull this into an update branch to do version and release note stuff. It'll publish when that merges.

@gregsdennis
Copy link
Owner

gregsdennis commented Apr 19, 2024

The copying is due to JsonNode only being allowed to have a single parent. It's generally necessary because we're likely pulling nodes from other JSON structures, but I don't like it. I could do some inspection to determine whether the specific situation calls for it, but... meh.

@gregsdennis gregsdennis merged commit b447356 into gregsdennis:jsone/update Apr 19, 2024
5 checks passed
@mi25iw
Copy link
Contributor Author

mi25iw commented Apr 19, 2024

Makes sense. Thanks for the quick response (and for the libraries in general)!

@mi25iw mi25iw deleted the fix/713-unknown-context-value branch April 19, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants