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

builtin: add pick(stream) #2656

Merged
merged 3 commits into from
Jul 6, 2023
Merged

builtin: add pick(stream) #2656

merged 3 commits into from
Jul 6, 2023

Conversation

pkoppstein
Copy link
Contributor

pick(stream) works on both arrays and objects

Restrictions on the dot-paths are specified in manual.yml

See #2578

pick(stream) works on both arrays and objects

Restrictions on the dot-paths are specified in manual.yml

See jqlang#2578
@pkoppstein pkoppstein requested a review from itchyny July 5, 2023 09:06
@wader
Copy link
Member

wader commented Jul 5, 2023

I wonder if it would make sense to extend the object construction syntax to allow things like {a, b.c, x} to mean {a: .a, b: {c: .b.c}, x: .x}? but would that also work for arrays somehow? would {a[1].b} mean? {a: [.a[1].b] } or {a: [null.a[1].b]}? and what would {a: [.a[1,2].b] } mean? should be in sync with destructing syntax 🤔

Sorry for rambling, maybe i should create a proposal issue to collect ideas if it's something we want to consider? ... also i have no idea about implementation difficulty

@pkoppstein
Copy link
Contributor Author

@wader - the linked issue already refers to such a proposal, which of course involves an extension to the language, whereas this PR only proposes a builtin, which I think is simple and useful enough to be included regardless of any such extension.

@wader
Copy link
Member

wader commented Jul 5, 2023

@pkoppstein Aha, i see. A nice thing with pick/1 is that things like this works pick(.. | select(...))

src/builtin.jq Outdated
@@ -260,6 +260,12 @@ def walk(f):
else f
end;

# stream should be a stream of dot-paths
def pick(stream):
Copy link
Contributor

@nicowilliams nicowilliams Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than call the thunk stream I'd call it paths or pathexp. After all, it's not used as a stream but rather passed to path/1 which then generates a stream of paths.

@@ -925,6 +925,24 @@ sections:
input: '{"a": 1, "b": 2, "c": 3}'
output: ['{"a": 2, "b": 3, "c": 4}']

- title: "`pick(stream)`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want s/stream/pathexp/ here and below, since the closure passed here is passed to path/1, so it's really a path expression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to update the docs to say pathexps.

@nicowilliams
Copy link
Contributor

Apart from proposing s/stream/pathexp/ for the name of the closure passed to pick/1, I like this a lot!

@pkoppstein
Copy link
Contributor Author

@nicowilliams wrote:

s/stream/pathexp/

How about s/stream/pathexps/ ? I wanted to emphasize the "reduction" aspect of the def, i.e. that the argument is NOT regular.

@nicowilliams
Copy link
Contributor

How about s/stream/pathexps/ ? I wanted to emphasize the "reduction" aspect of the def, i.e. that the argument is NOT regular.

Works for me!

Change the name of the formal parameter.
Change formal parameter name
@pkoppstein
Copy link
Contributor Author

@nicowilliams - Done. Thanks for reminder.

@nicowilliams nicowilliams merged commit c68ad08 into jqlang:master Jul 6, 2023
@nicowilliams
Copy link
Contributor

Thanks!

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.

4 participants