Skip to content

[1/n][sl][github][gh stacks] support headers and list params - #1392

Open
raydatray wants to merge 1 commit into
facebook:mainfrom
raydatray:pr1392
Open

[1/n][sl][github][gh stacks] support headers and list params#1392
raydatray wants to merge 1 commit into
facebook:mainfrom
raydatray:pr1392

Conversation

@raydatray

@raydatray raydatray commented Jul 31, 2026

Copy link
Copy Markdown

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)
  • mock_utils updated to match

test plan

doctests in _format_param (registered in test-doctest.py), existing github .t suite


Stack created with Sapling. Best reviewed with ReviewStack.

@meta-codesync

meta-codesync Bot commented Jul 31, 2026

Copy link
Copy Markdown

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.)

Comment on lines +116 to +119
if isinstance(value, list):
return list(
itertools.chain(*[_format_param(f"{key}[]", v) for v in value])
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if isinstance(value, list):
return list(
itertools.chain(*[_format_param(f"{key}[]", v) for v in value])
)
if isinstance(value, list):
if not value:
return ["-F", f"{key}[]"]
return list(
itertools.chain(*[_format_param(f"{key}[]", v) for v in value])
)

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

@lazopm

lazopm commented Aug 7, 2026

Copy link
Copy Markdown

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.

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.

3 participants