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

Batch commands #255

Open
Alizter opened this issue Oct 20, 2023 · 1 comment
Open

Batch commands #255

Alizter opened this issue Oct 20, 2023 · 1 comment

Comments

@Alizter
Copy link
Contributor

Alizter commented Oct 20, 2023

Now that we have a proper syntax for patch series 1-3, we can think about adding multiple arguments to certain commands in an unambiguous way from a parsing standpoint. The idea is that a gps command followed by multiple rather than a single series: gps command 1 2-3 4-7 should batch said commands to something like gps command 1; gps command 2-3; gps command 4-7.

This will subsume commands like gps batch-request-review.

Now there are some questions that we need to answer in order to fully develop this idea.

  1. How to do error handling?
  2. Which commands to these semantics make sense for?
  3. Is it intuitive to use?
  4. Is it useful?
@github-project-automation github-project-automation bot moved this to Triage in git-ps-rs Oct 20, 2023
@drewdeponte drewdeponte moved this from Triage to Exploration in git-ps-rs Oct 20, 2023
@drewdeponte drewdeponte added this to the 7.0.0 milestone Oct 20, 2023
drewdeponte added a commit that referenced this issue Oct 20, 2023
Add PatchIndexRangeBatch concept to represent a batch or collection of
PatchIndexRange objects. This also houses the parsing logic to
constitute a PatchIndexRangeBatch from a string representation of the
form "1 3-5 7 9 10-13".

In the near future I plan to use this to implement batching into the
request-review command so that I can get rid of the batch-request-review
command and reduce the command surface area.

This relates to issue #255.

<!-- ps-id: 21328cb6-0e2a-49ed-8134-d0a205675f01 -->
drewdeponte added a commit that referenced this issue Oct 20, 2023
Add batching support to the request-review command so that users would
be able to request review for a series of patches and patch series with
one go.

This also makes it so the request-review command supports the same
functionality as the batch-request-review command. Which will allow me
to now remove the batch-request-review command and reduce the surface
area of the commands so it isn't so overwhelming to new users.

This relates to issue #255.

[changelog]
added: batching support to the request-review command

<!-- ps-id: 524746ce-3a64-435d-8a7d-25149ad6be97 -->
drewdeponte added a commit that referenced this issue Oct 20, 2023
Remove batch-request-review command because the request-review command
now natively supports batching in a far more robust fashion.

Therefore, the batch-request-review command is no longer needed and is
just noise within the command surface area. Hence, why I am removing it
to streamline the command surface area and make the app feel less
overwhelming to newcomers.

This relates to issue #255.

[changelog]
removed: batch-request-review command

<!-- ps-id: 32bcc8f6-5d73-4c74-8b16-2f55f0047842 -->
@drewdeponte
Copy link
Owner

The parsing for this has been implemented and it has been integrated into the request-review command in mainline.

The behavior I implemented for handling errors with batched commands in request-review was to NOT have it short circuit the application. Instead it allows the other batched patch index ranges to run as well. I did this to maintain the behavior of the batch-request-review command which already had this behavior. I also think that this behavior makes sense for the request-review command as I don't believe previous patch index ranges should have any impact on the later ones requesting review.

However, even though we have the parsing available to all of the commands. I think that the behavior we want for other commands may be different. For example if we talk about integrate it can run a pull at the end of each integrate which would rebase the patch stack and then the patch index ranges wouldn't match correctly for the following patch index ranges.

So my point is that we likely just need to think through each of the cases where we want batching support and determine what type of behavior we want for that command when batching.

@drewdeponte drewdeponte removed this from the 7.0.0 milestone Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants