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

Add builtin function compact #2838

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

owenthereal
Copy link
Member

compact removes nulls from an object.

src/builtin.jq Outdated Show resolved Hide resolved
@wader
Copy link
Member

wader commented Aug 14, 2023

Implementation LGTM but i guess we have to decide if it goes into 1.7 or not? as i understood around the discussion about pick we wanted a compact but for arrays only but i could have misunderstood?

@nicowilliams
Copy link
Contributor

You have to also include a regen of jq.1.prebuilt in order for the checks to pass.

@owenthereal
Copy link
Member Author

@wader:

Implementation LGTM but i guess we have to decide if it goes into 1.7 or not? as i understood around the discussion about pick we wanted a compact but for arrays only but i could have misunderstood?

I personally think that it's fine to go into 1.7 unless we think 1.7 should have zero new feature. compact could be the building blocks for other stuff as well besides improving the existing pick implementation.

@nicowilliams:

You have to also include a regen of jq.1.prebuilt in order for the checks to pass.

Fixed in 04a0dc5

This PR is ready for review again 😄

@pkoppstein
Copy link
Contributor

pkoppstein commented Aug 27, 2023

@owenthereal wrote:

compact could be the building blocks for other stuff as well besides improving the existing pick implementation.

I am not sure whether the above refers to the behavior of pick on arrays or objects, but in either case, I would like to point out that the way pick inserts nulls in both cases is both intended and (at least in part) documented.

Consider in particular the fact that pick will add "null" values to object inputs, e.g. {} | pick(.x) produces {"x": null}.

This behavior fits in properly with the jq scheme of things, as evidenced for example by the way it flows from the simplicity of the def of pick. In this connection, it should also be remembered that, at least from the point of view of jq's .x selector, the absence of a key named "x" is equivalent to a null-valued key named "x". Thus pick(.x) can be understood as an imperative: evaluate .x and ensure the result is reflected in the output.

On the other hand, it might make sense for keep/1 (as being developed by @nicowilliams) to avoid inserting nulls into arrays and objects. Should {} | keep(.x) raise an exception?

@pkoppstein
Copy link
Contributor

@wader wrote:

we have to decide if it goes into 1.7 or not?

My thinking is that it shouldn't, partly because we're already at jq 1.7rc2, but mainly because there hasn't been much discussion about it. Should we have guidelines as to when new trivial builtins should be added? In any case, it is very specific: in my experience, different users typically want to delete various combinations of distinguished values besides null, notably [], "", and/or other strings signifying the absence of a value.

@nicowilliams nicowilliams added this to the 1.8 release milestone Aug 27, 2023
@emanuele6
Copy link
Member

Personally, I think just del(.. | nulls) is easier to understand; do we want to merge this alias anyway?

@nicowilliams
Copy link
Contributor

Personally, I think just del(.. | nulls) is easier to understand; do we want to merge this alias anyway?

I tend to agree. I'm not sure I'd know what compact would do without first looking at the docs. I mean, that's often the case with jq builtins, but maybe we should try not to add more of these? The changes look ok besides that.

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.

None yet

6 participants