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

simplify paths/0 and paths/1 #2946

Merged
merged 1 commit into from
Nov 16, 2023
Merged

simplify paths/0 and paths/1 #2946

merged 1 commit into from
Nov 16, 2023

Conversation

asheiduk
Copy link
Contributor

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 later.

`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.
@emanuele6
Copy link
Member

emanuele6 commented Nov 15, 2023

Could even use .. instead of recurse.
.. is syntax sugar that compiles to a call to recurse/0.

@emanuele6 emanuele6 added the lint Code cleanup; style fixes; small refactors; dead code removal; etc. label Nov 15, 2023
@nicowilliams
Copy link
Contributor

LGTM.

@nicowilliams
Copy link
Contributor

@emanuele6 did you want to require the use of ..?

Copy link
Member

@wader wader left a 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?

@asheiduk
Copy link
Contributor Author

@wader There is something in man.test:

[paths(type == "number")]
[1,[[],{"a":2}]]
[[0],[1,1,"a"]]

@emanuele6
Copy link
Member

emanuele6 commented Nov 15, 2023

@emanuele6 did you want to require the use of ..?

Well, I thought it would look neater with .....

Also, it would be easier to read since path(..) is a common pattern, and .. is commonly used, while recurse with no arguments is basically never used, but fine either way I guess.

@asheiduk
Copy link
Contributor Author

@emanuele6 did you want to require the use of ..?

Well, I thought it would look neater with .....

Also, it would be easier to read since path(..) is a common pattern, and .. is commonly used, while recurse with no arguments is basically never used, but fine either way I guess.

@emanuele6 I think this is true for "user" code but here the definition of recurse is in the same file -- a file internal to jq itself. Having this textual direct link (e.g. via double-clicking) seemed more important to me. But that's only my take.

After all: Shall I change it so the PR can be merged or was this only an optional suggestion?

@emanuele6
Copy link
Member

Well, it is fine with recurse

@emanuele6 emanuele6 merged commit 88f01a7 into jqlang:master Nov 16, 2023
28 checks passed
@emanuele6
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lint Code cleanup; style fixes; small refactors; dead code removal; etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants