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

Optionally limit the number of PRs created in each submission #824

Closed
wants to merge 2 commits into from

Conversation

rejc2
Copy link
Contributor

@rejc2 rejc2 commented Jan 23, 2024

Optionally limit the number of PRs created in each submission

Currently, when submitting a PR in Sapling, it's easy to accidentally create multiple PRs when only one was intended (e.g. forgetting to run sl pr follow, or editing a PR that is based off a different branch).

With this change, you can configure Sapling to limit the number of PRs that can be created in a single submit action, e.g. to just 1:

sl config --user github.max-prs-to-create=1

Now when attempting to create multiple PRs in a single sl pr submit action, Sapling will fail with:

abort: refused to create 2 pull requests, max is 1
if you want to create 2 pull requests at once, run again with `--config github.max-prs-to-create=-1`

(Note that this setting doesn't limit the size of a PR stack: a larger PR stack can be made by submitting each new PR separately. Re-submitting the top PR will still update the entire stack below it, no matter the size.)

Based on feedback and discussion, I've set the default for github.max-prs-to-create to be 5.

Currently, when submitting a PR in Sapling, it's easy to accidentally create multiple PRs when only one was intended (e.g. forgetting to run `sl pr follow`, or editing a PR that is based off a different branch).

With this change, you can configure Sapling to limit the number of PRs that can be created in a single submit action, e.g. to just 1:
```
sl config --user github.max-prs-to-create=1
```
Now when attempting to create multiple PRs in a single `sl pr submit` action, Sapling will fail with:
```
abort: refused to create 2 pull requests, max is 1
```
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rejc2 rejc2 marked this pull request as ready for review January 23, 2024 17:54
Copy link
Contributor

@vegerot vegerot left a comment

Choose a reason for hiding this comment

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

As a follow up to this commit, I'd suggest adding a default limit of maybe 10, and update the error message to instruct users how to override it.

Comment on lines 181 to 182
_("refused to create %d pull requests, max is %d")
% (len(params.pull_requests_to_create), max_pull_requests_to_create)
Copy link
Contributor

@vegerot vegerot Jan 24, 2024

Choose a reason for hiding this comment

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

nit:

Suggested change
_("refused to create %d pull requests, max is %d")
% (len(params.pull_requests_to_create), max_pull_requests_to_create)
_("refused to create %d pull requests, max is %d\nif you want to open %d pull requests, run again with `--config github.max-prs-to-create=-1")
% (len(params.pull_requests_to_create), max_pull_requests_to_create, len(params.pull_requests_to_create) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!
I've updated the PR, and (following our discussion on Discord) set the default value to 5.

Still open to further feedback of course.

Also update error message to explain how to create more than 5 PRs in one command.
@facebook-github-bot
Copy link
Contributor

@rejc2 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@vegerot vegerot left a comment

Choose a reason for hiding this comment

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

❤️

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in e62f3e8.

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