replace_with call back function should be able to return an Error

[freestrings]

replace_with call back function should be able to return an Error

[gkorland]

May be an Option of a Result ?
https://doc.rust-lang.org/rust-by-example/error/multiple_error_types/option_result.html

pub fn try_replace_with<F>(value: Value, path: &str, fun: &mut F) -> Result<Value, JsonPathError>
where
    F: FnMut(&Value) -> Option<Result<Value, JsonPathError>>,

[freestrings]

looks good.

[gkorland]

On the same topic I think it can also be useful to just do nothing.
Meaning if the callback code decides it doesn't want to make any effect it should be able to indicates it.
Currently, if the callback wants to maintain the same value it needs to clone it and return the cloned value.
That is how we've done it https://github.com/RedisLabsModules/RedisDoc/blob/4dc407d4682ce4b5569342c7136e7cffec68c153/src/redisjson.rs#L144

[freestrings]

How about Option instead of Result?

None: skip,
Some: replace

pub fn try_replace_with<F>(value: Value, path: &str, fun: &mut F) -> Result<Value, JsonPathError>
where
    F: FnMut(&Value) -> Option<Value>,

[gkorland]

Second thought maybe we should F to get the Value and not a reference.

pub fn try_replace_with<F>(value: Value, path: &str, fun: &mut F) -> Result<Value, JsonPathError>
where
    F: FnMut(Value) -> Result<Value, JsonPathError>,

This way it will be able to return the same Value if no change needed or the and Error

[freestrings]

The second example looks better to me.

[freestrings]

@gkorland

Since the Value has been moved to F, in order to abort all changes, it need the cloned Value too. i think that rollback this commit 8b85ec9 or drop this issue.

Do you have a better idea?

[gkorland]

I think I prefer to drop this issue for now