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.

Sign up for GitHub

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

Jump to bottom

support pick(last); add to jq.test #2717

Closed
wants to merge 8 commits into from
Closed

support pick(last); add to jq.test #2717

wants to merge 8 commits into from

Conversation

pkoppstein
Copy link
Contributor

Modify last/0 to allow e.g. pick(last) (q.v. #2716)

Include some tests for input, inputs, debug, debug(), pick()

Also some tidying of spacing in builtin.jq

@pkoppstein
redefine last/0; add to jq.test
8e7bd77 
Modify last/0 to allow e.g. pick(last)

Include some tests for input, inputs, debug, debug(), pick()

Also some tidying of spacing in builtin.jq
emanuele6
emanuele6 reviewed Jul 15, 2023
View reviewed changes
tests/jq.test Show resolved Hide resolved
tests/jq.test Outdated Show resolved Hide resolved
emanuele6 added a commit to emanuele6/jq-1 that referenced this pull request Jul 15, 2023
@emanuele6
Initialise jq.input_cb{,_data} to NULL in jq_init().
fdc41de 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called, like when
using  jq --run-tests.

Ref: jqlang#2717 (comment)
emanuele6 added a commit to emanuele6/jq-1 that referenced this pull request Jul 15, 2023
@emanuele6
Initialise jq.input_cb{,_data} to NULL in jq_init().
05dcabd 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called, like when
using  jq --run-tests.

Ref: jqlang#2717 (comment)
emanuele6 added a commit to emanuele6/jq-1 that referenced this pull request Jul 15, 2023
@emanuele6
Initialise jq.input_cb{,_data} to NULL in jq_init().
8425388 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called; e.g. the one
used by  jq --run-tests.

That segfault could also by fixed in run_jq_tests() by calling:

  jq_set_input_cb(jq, NULL, NULL);

But I think it makes sense to just make jq_init() initialise those
values to NULL.

Ref: jqlang#2717 (comment)
emanuele6 added a commit to emanuele6/jq-1 that referenced this pull request Jul 15, 2023
@emanuele6
Initialise jq.input_cb{,_data} to NULL in jq_init()
10e3857 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called; e.g. the one
used by  jq --run-tests.

That segfault could also by fixed in run_jq_tests() by calling:

  jq_set_input_cb(jq, NULL, NULL);

But I think it makes sense to just make jq_init() initialise those
values to NULL.

Ref: jqlang#2717 (comment)
@emanuele6 emanuele6 mentioned this pull request Jul 15, 2023
Merged
emanuele6 added a commit to emanuele6/jq-1 that referenced this pull request Jul 15, 2023
@emanuele6
Initialise jq_state->input_cb{,_data} to NULL in jq_init()
f78c036 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called; e.g. the one
used by  jq --run-tests.

That segfault could also be fixed in run_jq_tests() by calling:

  jq_set_input_cb(jq, NULL, NULL);

But I think it makes sense to just make jq_init() initialise those
values to NULL.

Ref: jqlang#2717 (comment)
@pkoppstein pkoppstein mentioned this pull request Jul 16, 2023
Closed
@pkoppstein pkoppstein changed the title redefine last/0; add to jq.test support pick(last); add to jq.test Jul 16, 2023
nicowilliams pushed a commit that referenced this pull request Jul 16, 2023
@emanuele6 @nicowilliams
Initialise jq_state->input_cb{,_data} to NULL in jq_init()
e79335e 
To avoid causing segmentation faults when  input/1  is called in a
jq_state on which  jq_set_input_cb()  has not been called; e.g. the one
used by  jq --run-tests.

That segfault could also be fixed in run_jq_tests() by calling:

  jq_set_input_cb(jq, NULL, NULL);

But I think it makes sense to just make jq_init() initialise those
values to NULL.

Ref: #2717 (comment)
@pkoppstein
Copy link
Contributor Author

@nicowilliams @emanuele6 - I've merged this with e79335e
and run all the *.test test, so I believe this PR is ready for re-review.

@nicowilliams
Copy link
Contributor

I force pushed a rebase and re-arranging of the commits. I dropped all the restyling of src/builtin.jq -- that should be done in a separate PR (do we have a style guide for jq code? we probably should).

@itchyny
Copy link
Contributor

itchyny commented Jul 27, 2023

This PR is unable to merge due to too many unrelated coding style fixes.

@pkoppstein
Copy link
Contributor Author

pkoppstein commented Jul 27, 2023 •
edited
Loading

@itchyny - nicowilliams wrote:

I force pushed a rebase and re-arranging of the commits. I dropped all the restyling of src/builtin.jq

Can't you get the necessary commits from what he's done?

I've closed this PR i.f.o. #2779

Thanks.

@emanuele6
Copy link
Member

@pkoppstein
You have restored the style changes @nicowilliams removed with a merge commit, an hour after he removed them. If that was accidental, I can revert that merge commit for you if you want.

@pkoppstein pkoppstein mentioned this pull request Jul 27, 2023
Merged
@pkoppstein
Copy link
Contributor Author

@emanuele6 - Thanks for your offer, but in the meantime, I created another PR (#2779),
which is based on the most recent jqlang, so I'll close this PR, and hope for the best.

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

@itchyny - nicowilliams wrote:

I force pushed a rebase and re-arranging of the commits. I dropped all the restyling of src/builtin.jq

Can't you get the necessary commits from what he's done?

The problem is that after I force-pushed you then did a git merge and pushed again.

I recommend that you break out of the merge workflow. Merging is a crutch you don't need. Every time you want to say git pull --merge, or git pull, or git merge, you should imagine a kitten being mauled by a dog because of you doing that, then remember to try git pull --rebase, or git fetch origin then git rebase origin/master instead so that the kitten doesn't die.

Anyways, I get permission denied trying to push to this PR now. What you should do:

$ # review what I pushed:
$ git log --patch --reverse $(git merge-base origin/master 8f5d3aee13128c169ace637e06ce3fd4405d1960)..8f5d3aee13128c169ace637e06ce3fd4405d1960
$ # if you agree to what I pushed, then:
$ git checkout tests-jq-test
$ git branch save-this-branch HEAD # so you don't lose your work
$ git reset --hard 8f5d3aee13128c169ace637e06ce3fd4405d1960 # this is what I force pushed
$ git push -f $your_repo_name_here tests-jq-test

then we can merge this.

@pkoppstein
Copy link
Contributor Author

@nicowilliams wrote:

Merging is a crutch you don't need.

These days, I'm trying to steer clear of the git command line in favor of GitHub Desktop. There's an option there to "Update from master", which I've sometimes used on a branch. Is that in effect what you're advising against?

@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 28, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emanuele6 emanuele6 emanuele6 left review comments

@itchyny itchyny Awaiting requested review from itchyny

@nicowilliams nicowilliams Awaiting requested review from nicowilliams

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pkoppstein @nicowilliams @itchyny @emanuele6