Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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!

# for free to join this conversation on GitHub. Already have an account? # 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