facebook
PR description in `pr submit`
I've been rather enjoying using Sapling, it seems quite close to my ideal workflow, but I'm wondering if it would be possible to change the text that is generated when submitting a PR with sl pr submit.
I'd ideally like the actual change in the PR to be front and center, and my personal choice of tools to be less intrusive for others that might not use them. Right now the stack description is always present, even if the stack only has one PR, and the current PR description is at the end.
Current formatting:
Stack created with Sapling. Best reviewed with ReviewStack.
- -> Current pr summary
- Other pr summary
Description of current PR
I'm thinking alternate formatting could be something like this:
Description of current PR
This PR is part of a stack. View current diff on GitHub or ReviewStack. PRs in the stack (via Sapling):
- -> Current pr summary
- Other pr summary
The idea is that the GitHub link links to a diff for only the commits part of the current pr, not the entire stack (I know that you can view that and make a link to it, but I'm presently not sure how to do it automatically), so as to not require others to use an external tool. I'm also thinking that everything after the horizontal line would only be included if the stack contains at least two PRs. Presumably it would then be added if a stack goes from one PR to two before the first is merged.
I realize that this is likely to be a fairly subjective matter, so I imagine this would be an option somewhere via sl config. When searching for where the message was produced I also found what I assume is the code in ReviewStack that parses the stack, so a corresponding change would also be required there I imagine.
If I were to look at implementing something like this, would it be interesting to merge? If not, I assume I could use it myself by having a forked version of the github extension and pointing to it in the config?
I think introducing the use of a horizontal rule and putting the stack information at the bottom is quite reasonable.
The problem with making the thing arbitrary customizable is that ReviewStack needs to be able to parse stack information reliably. The current checks that the body starts with Stack created with [Sapling], so I'll have to change that to support the horizontal rule case:
https://github.com/facebook/sapling/blob/c304aa820de6e4307c268c403f98f4b5f52510bc/addons/reviewstack/src/saplingStack.ts#L57
What do you think about keeping the "line that starts with Stack created with [Sapling]" heuristic, but then making the rest of that line customizable? Though it gets tricky quickly if you want to have links to both GitHub and ReviewStack since then your template string needs to be paramaterizable...
I figure it's useful to look at the extremes, to see where the design space is, so here's the most flexible scheme I can think of atm that still seems somewhat plausible to implement. Feel free to push back on points here, again, it's just to get an idea of the design space.
- Make the PR description be generated from a custom template string (i.e., a new option in
sl config) with a couple of spliceable values (e.g.,{description}):- Commit description
- Commit summary
- Reviewstack link (just the URL, not the
[]()stuff around) - Github diff link (just the URL, not the
[]()stuff around) - The bullet list of PRs in the current stack (just the bullets, not the
Stack created with [Sapling]header)
- Maybe have a separate template string for the "stack of one PR" case, but that doesn't feel particularly elegant, and there might be more cases where one would want other templates, so it's not necessarily the most generalizable
- ReviewStack would see a PR as a stack if it contains an appropriately formatted bullet list, i.e.:
- One or more adjacent bullets containing only a link to a PR
- One of the bullets has the link preceeded by an arrow, and the link goes to the PR we're parsing
- The bullets are not indented, not quoted, not inside a code block, etc.
Of course, this heuristic for figuring out if a PR is a stack has some pros and cons:
- Pro: other tools could generate PRs that reviewstack can read as a stack, without having to lie about being generated by Sapling
- Con: to be able to truly rely on this the format would have to be locked, which might not be desirable
- Pro: the segment of the generated message that has to remain stable to not break ReviewStack is smaller
- Pro: the new format is backwards compatible, PRs that were previously recognized by ReviewStack should still be recognized (even in weird edge-cases, as long as the first valid list is the one picked).
- Con: it is more expensive; to get it fully correct ReviewStack would have to parse markdown and then examine the result, rather than just check if a known string is present at the beginning of the body. Of course most cases would be handled properly by looking for sequences of consecutive lines that start with exactly
*, then checking if any of those follows the appropriate format, since it seems pretty rare that there would accidentally be an appropriately formatted list within a code block or something similar.
I also tried to think a little bit about #256, but it doesn't seem obvious to me where one would splice the Sapling generated message into an arbitrary GitHub template. On the other hand, if there is a new option for template in sl config, then that could be edited to have a modified version of the GitHub template. I guess the friendliest behavior I can think of here is to detect that a repository has a template when the repo is cloned (or when the first PR is created), then open an $EDITOR containing the GitHub template and some comments describing the default Sapling template and the spliceable values available, and ask the user how to insert what Sapling generates into it.
On a separate note, I believe this is how one would generate the link to view the appropriate diff on GitHub:
- If the current PR is the first in the stack:
-
https://github.com/<owner>/<repo>/pull/<pr-number>/files/<commit-hash-of-last-commit-in-PR> - or just
https://github.com/<owner>/<repo>/pull/<pr-number>/files/, since all commits are relevant in the first PR.
-
- Otherwise:
-
https://github.com/<owner>/<repo>/pull/<pr-number>/files/<commit-hash-of-last-commit-in-previous-PR>..<commit-hash-of-last-commit-in-PR>, i.e., the range excludes the first commit and includes the last.
-
I should note that for GitHub in particular, you can use <!-- html comments --> and they will be hidden like you expect. So a more robust method of detection in the long run might actually be to wedge all of the data in like:
This is my big description...
---
<!-- BEGIN: SAPLING MARKVER v2 -->
Stack created with [Sapling]
<!-- END: SAPLING MARKER v2 -->
In the above example, even the format of the syntax that ReviewStack has to parse is included (v2), so you could gradually roll out/support multiple versions with clearer intent.
