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

revamp walk/1 #2655

Closed
wants to merge 62 commits into from
Closed

revamp walk/1 #2655

wants to merge 62 commits into from

Conversation

pkoppstein
Copy link
Contributor

@pkoppstein pkoppstein commented Jul 5, 2023

Resolves #2584; also resolves #2611

@pkoppstein pkoppstein requested a review from itchyny July 5, 2023 06:02
@itchyny
Copy link
Contributor

itchyny commented Jul 5, 2023

The output of {x:0} | walk(.,1) changes by this PR. Is it intended?

@pkoppstein
Copy link
Contributor Author

It is not the primary intention, which was to make the definition in terms of map and map_values as simple as possible while ensuring correct behavior when the arg was "simple" (no top-level commas). The current behavior was unintended.

Perhaps the manual should mention this example?

@itchyny
Copy link
Contributor

itchyny commented Jul 5, 2023

I just wanted to confirm that you're aware of the behavior change. Even if not mentioned in the manual, we need reasonable explanations to any behavior changes. Typical reason is that the original behavior was unintentional bug, but making the implementation simpler is also a good reason. This time, I think changing this behavior would be Okay. By the way, in gojq I use last to make it work like jq but I will drop it...

@nicowilliams
Copy link
Contributor

Hmm, I think we want the original behavior, yeah.

@nicowilliams
Copy link
Contributor

Or maybe I should read @2584 before commenting...

@pkoppstein
Copy link
Contributor Author

@nicowilliams - Perhaps it would help to review the situation w.r.t. walk/1.

  1. The old def of walk(f) was written without regard to the
    possibility that f could be a sequence of expressions. I have not
    double-checked but hopefully the official documentation
    never came remotely close to specifying what the semantics should be
    in such cases.

  2. In the case in question, I think it's generally agreed that one would want

{x:0} | walk(.,1) to be equivalent to {x:0} | walk(.), walk(1)

In this and other cases, jaq and the new walk are in agreement,
and as you have no doubt noticed,  @itchyny indicated that he had to go out of his way
to align gojq's `walk` behavior with jq's.
  1. The new def elegantly resolves several important documented issues; this consideration,
    in conjunction with the fact that the new def of walk/1 is as close to a "declarative" specification of
    the desired functionality as one could achieve or want in jq, seem to me very compelling.

wtlangford and others added 20 commits July 21, 2023 09:01
These are stored in vars, so we need to make sure that nlocals is large
enough to account for that
Close jqlang#1885, jqlang#2140, jqlang#2011, jqlang#2220, jqlang#2485, jqlang#2073

Rename the FORK_OPT opcode to TRY_BEGIN, add a TRY_END opcode, and wrap
errors when raising through a TRY_END so that they will not be caught by
the matching TRY_BEGIN.

Now a `try exp catch handler` expression generates code like:

    TRY_BEGIN handler
    <exp>
    TRY_END
    JUMP past_handler
    handler: <handler>
    past_handler:
    ...

On backtrack through TRY_BEGIN it just backtracks.

If anything past the whole thing raises when <exp> produced a value,
then the TRY_END will catch the error, wrap it in another, and
backtrack.  The TRY_BEGIN will see a wrapped error and then it will
unwrap and re-raise the error.

If <exp> raises, then TRY_BEGIN will catch the error and jump to the
handler, but the TRY_BEGIN will not stack_save() in that case, so on
raise/backtrack the TRY_BEGIN will not execute again (nor will the
TRY_END).
…c.) (jqlang#2744)

- Add error/0 and mentions null input behavior (close jqlang#2231)
- Explain value iterator suffix syntax .foo[] (close jqlang#1047)
- Mention array slicing is also zero-based (close jqlang#2094)
- Add examples of input and inputs filters (close jqlang#2216, close jqlang#2470)
- Improve sort_by about multiple values (close jqlang#2103, close jqlang#2467, close jqlang#2474)
- Improve foreach section and simplify examples (close jqlang#1148, close jqlang#2169)
- Fix recurse/1 document on how it is identical using recurse/2 (close jqlang#2036, close jqlang#2412)
- Add non-string examples of index/1, rindex/1 (close jqlang#1422)
- Simplify the example of truncate_stream/1 (close jqlang#1736)
Signed-off-by: David Korczynski <david@adalogics.com>
itchyny and others added 17 commits July 27, 2023 17:01
Changes mentioned based on picking user facing changes from:
git log --oneline -r master...jq-1.6 | grep -v Merge
correct grammar, add attributions, clarify abs
Backfill with references to PRs & issues in NEWS.md
We now have an official Discord server and most maintainers are hanging
out there. It would be a good idea to redirect questions to Discord.
* Bump up Bootstrap to v5.3.1, Bootstrap Icon to v1.10.5.
* Use autoComplete.js to drop dependency on jQuery and typeahead.js.
* Support dark mode.
* New svg logo and icon with responsive color mode support.
* Normalize section ids to lower kebab-case for easiness of linking.
* Use relative paths for links for local development (--root /output).
* Various markup cleanups and accessibility improvements.
src/builtin.jq Outdated
@@ -249,17 +249,18 @@ def bsearch($target):
else .[2]
end
end;

#
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this line to be an empty line rather than an empty comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but someone should probably run: s/^# *$//

@itchyny
Copy link
Contributor

itchyny commented Jul 31, 2023

Please rebase this PR, thanks.

Note that according to the new def:

`{x:0} | walk(.,1)` is equivalent to: {x:0} | walk(.), walk(1)
@pkoppstein
Copy link
Contributor Author

@itchyny wrote:

Please rebase this PR, thanks.

I did, but somehow a "merge" was added as well. Feel free to "squash" as desired.

@itchyny
Copy link
Contributor

itchyny commented Jul 31, 2023

Could you rebase the commit onto the latest remote master branch? This PR includes the commits ahead and I can't review the diff now.

pkoppstein added a commit to pkoppstein/jq that referenced this pull request Jul 31, 2023
Resolves jqlang#2584; also resolves jqlang#2611
and supersedes jqlang#2655

Note that according to the revised implementation:

`{x:0} | walk(.,1)` is equivalent to `{x:0} | walk(.), walk(1)`
@pkoppstein
Copy link
Contributor Author

Given the difficulty merging this PR, I'm closing it in favor of #2795

@pkoppstein pkoppstein closed this Jul 31, 2023
@nicowilliams
Copy link
Contributor

@pkoppstein if you would only just use your terminal application and the git command you wouldn't have such difficulties and you wouldn't cause so many duplicate PRs.

@pkoppstein
Copy link
Contributor Author

@nicowilliams wrote:

use your terminal application and the git command

In brief, I did, but itchyny said he could not use the result of a simple "Rebase" and "Push".

In long - well, let me know if and when you would like further details.

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.

ER: simpler, faster walk/1 Deleting objects using walk has side effects
10 participants