-
Notifications
You must be signed in to change notification settings - Fork 150
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
fix: use --slurp with --paginate to concat pages into valid json #820
base: master
Are you sure you want to change the base?
Conversation
Fixes: pwntester#819 The original arguments to `gh api` for certain commands are problematic. For instance: @@ -85,13 +95,14 @@ end function PullRequest:get_changed_files(callback) local url = string.format("repos/%s/pulls/%d/files", self.repo, self.number) gh.run { - args = { "api", "--paginate", url, "--jq", "." }, In the above arguments the returned results, if exceeds one pages, will be two separate JSON arrays. E.g. [{page1..}][{page2...}]. You cannot feed this directly to `vim.fn.jsondecode` since its not actually valid json, its two json arrays in a single string. Luckily, `gh` includes the `--slurp` argument to the `api` command which works similiarly to `jq`'s slurp command, concating sequential arrays. We can use this, with one catch. The results of any API call with `--slurp` is "an array of pages" and thus the data we want is nested one level deep. We simply write a `merge_pages` function which extracts all items into a 1d array, which is what Octo currently expects. Signed-off-by: ldelossa <[email protected]>
hmm, this could be seen as a breaking change, but only if you're using a I'll look for advice on how to handle that. |
Thanks for the PR @ldelossa Let me take a look at the code. As for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this function already exists
cb = function(output, stderr) | ||
if stderr and not utils.is_blank(stderr) then | ||
utils.error(stderr) | ||
elseif output then | ||
local FileEntry = require("octo.reviews.file-entry").FileEntry | ||
local results = vim.fn.json_decode(output) | ||
results = merge_pages(results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this new function can be replaced with this current one?
Lines 782 to 796 in 5579f20
function M.aggregate_pages(text, aggregation_key) | |
-- aggregation key can be at any level (eg: comments) | |
-- take the first response and extend it with elements from the | |
-- subsequent responses | |
local responses = M.get_pages(text) | |
local base_resp = responses[1] | |
if #responses > 1 then | |
local base_page = M.get_nested_prop(base_resp, aggregation_key) | |
for i = 2, #responses do | |
local extra_page = M.get_nested_prop(responses[i], aggregation_key) | |
vim.list_extend(base_page, extra_page) | |
end | |
end | |
return base_resp | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If im not mistaken those are used for "graphql" queries and the string returned from one. This is not the same as getting paged JSON from "gh api" (used without the graphql flag).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no im wrong!
I think youre right. That function can be used (as long as it doesnt require a merge key at all). It looks like the alternative to "--slurp".
Maybe, to fix the linked issue, we actually just need to use that function for the api queries that are currently broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give it a try. Not sure the differences of yours and what is currently there. Just noticed some patterns of use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, stay tuned 📺
Additionally, if it does work, it would be because "--slurp" isnt being used, it basically splits the raw json string by "\n". Now that "--slurp" exists, maybe its better to move Octo to use this? Just to reduce the need to do the manual string manipulation before handling pages.
Fixes: #819
The original arguments to
gh api
for certain commands are problematic.For instance:
In the above arguments the returned results, if exceeds one pages, will be two separate JSON arrays. E.g. [{...}][{...}].
You cannot feed this directly to
vim.fn.jsondecode
since its not actually valid json, its two json arrays in a single string.Luckily,
gh
includes the--slurp
argument to theapi
command which works similiarly tojq
's slurp command, concating sequential arrays.We can use this, with one catch.
The results of any API call with
--slurp
is "an array of pages" and thus the data we want is nested one level deep.We simply write a
merge_pages
function which extracts all items into a 1d array, which is what Octo currently expects.Describe what this PR does / why we need it
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews
Checklist