Uh oh!
There was an error while loading. Please reload this page.
[1/n][sl][github][gh stacks] support headers and list params - #1392
[1/n][sl][github][gh stacks] support headers and list params#1392raydatray wants to merge 1 commit into
Conversation
This pull request has been imported. If you are a Meta employee, you can view this in D114402510. (Because this pull request was imported automatically, there will not be any future comments.) |
| if isinstance(value, list): | ||
| return list( | ||
| itertools.chain(*[_format_param(f"{key}[]", v) for v in value]) | ||
| ) |
There was a problem hiding this comment.
| ifisinstance(value, list): | |
| returnlist( | |
| itertools.chain(*[_format_param(f"{key}[]", v) forvinvalue]) | |
| ) | |
| ifisinstance(value, list): | |
| ifnotvalue: | |
| return ["-F", f"{key}[]"] | |
| returnlist( | |
| itertools.chain(*[_format_param(f"{key}[]", v) forvinvalue]) | |
| ) |
I have not finished reading the stack, but gh api states "To pass an empty array, use key[] without a value." (https://cli.github.com/manual/gh_api)
if we're formatting a empty list, we should do so explicitly rather than dropping the param and not sending it since it seems like "empty array" and "missing field" can have different behaviors downstream
There was a problem hiding this comment.
hi Genevieve! thanks for taking the time to take a look, sorry for the turn around time work has been busy. i've taken another sweep at the stack and applied your suggestion
I am really excited for this to land! after leaving Meta having diff stacks is something I miss every day. If we get this right it might be the thing that pushes SL more mainstream. Today it's hard for me to sell sapling to my team since the workflow generates unreadable massive PRs unless you're diligent about managing the stack, and using ReviewStack on top just feels hacky vs something native to github. @raydatray any chance you could upload a screencast of the workflow in your test plan? In fact, can we make THIS stack use the github stacked PRs feature? I hope we can expedite merging this behind an experimental setting flag. @genevievehelsel@raydatray I'm writing a ton of PRs across 2-3 repos with sapling, happy to beta test and file/fix bugs as I find them. |
@raydatray has updated the pull request. You must reimport the pull request before landing. |
unfortunately since the stack feature doesnt support forks, i cant make show off the feature here. but in the subsequent diffs in the test plan i do show screenshots of me using the feature. i see that youve also done your own implementation, if you have more free time than me, i am more than happy handing this feature off to you and dogfooding it edit: this diff shows step by step the feature #1397 |
### ctx
github added native stacked pull requests, it would be nice to add native support to it for sapling!
this stack lets `sl pr submit` and `sl pull` via a new `github.pr.workflow = stacked` mode
### changes made
- `github_gh_cli.make_request` accepts custom headers (`-H`, needed for the `X-GitHub-Api-Version` preview header)
- `_format_param` supports list values using the `gh api` repeated-field syntax (`pull_requests[]=101`)
- an empty list is passed explicitly as `key[]` without a value (per the gh api manual) rather than dropped, since an empty array and a missing field can mean different things to an endpoint
- mock_utils: `MockGitHubServer` now tracks which expectations were consumed; tests can opt in via `wrap_with_consumption_check` to fail when an expected request silently stops happening (closes the old TODO)
### test plan
doctests in `_format_param` (registered in test-doctest.py, including the empty-array case), existing github .t suite. verified the empty-array wire format empirically: `gh api -F "pull_requests[]" --verbose` sends `{"pull_requests": []}`@raydatray has updated the pull request. You must reimport the pull request before landing. |
ctx
github added native stacked pull requests, it would be nice to add native support to it for sapling!
this stack lets
sl pr submitandsl pullvia a newgithub.pr.workflow = stackedmodechanges made
github_gh_cli.make_requestaccepts custom headers (-H, needed for theX-GitHub-Api-Versionpreview header)_format_paramsupports list values using thegh apirepeated-field syntax (pull_requests[]=101)key[]without a value (per the gh api manual) rather than dropped, since an empty array and a missing field can mean different things to an endpointMockGitHubServernow tracks which expectations were consumed; tests can opt in viawrap_with_consumption_checkto fail when an expected request silently stops happening (closes the old TODO)test plan
doctests in
_format_param(registered in test-doctest.py, including the empty-array case), existing github .t suite. verified the empty-array wire format empirically:gh api -F "pull_requests[]" --verbosesends{"pull_requests": []}Stack created with Sapling. Best reviewed with ReviewStack.