-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
simplify paths/0 and paths/1 #2946
Conversation
`recurse/0` already handles traversing objects and arrays, so it is more consistent to use that. For `paths/1` it is easier to use the actual value returned by ` recurse` instead of querying that value with `getpath/1` afterwards.
Could even use |
LGTM. |
@emanuele6 did you want to require the use of |
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.
LGTM but i noticed there is no tests for paths/1
, time to add?
@wader There is something in
|
Well, I thought it would look neater with Also, it would be easier to read since |
@emanuele6 I think this is true for "user" code but here the definition of After all: Shall I change it so the PR can be merged or was this only an optional suggestion? |
Well, it is fine with |
Thank you! |
recurse/0
already handles traversing objects and arrays, so it is more consistent to use that.For
paths/1
it is easier to use the actual value returned byrecurse
instead of querying that value withgetpath/1
later.