-
Notifications
You must be signed in to change notification settings - Fork 7
feat: non-destructive updates to PR body #51
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
feat: non-destructive updates to PR body #51
Conversation
…her list in the body
|
@Eric162 Thanks for taking the time to create this PR! This is definitely a glaring bug that needs to be fixed! With this fix, I get the sense that we're still going to have edge cases that haven't been discovered yet. The root problem is that we're having to make assumptions about the position of the stack visualisation in the AST because there's no deterministic method at the moment. I think the ideal solution would be one that relies determinism. What if we added an anchor comment to the root node of the graph? For example, something like this: That way, instead of assuming that the the list will be the node immediately after the anchor, we can instead traverse the AST and find a list node with |
|
Hey @tranhl thanks for the response - i actually have another proposal in my local repo as well to improve the reliability here, but just didn't wanna break anyone: https://github.com/Eric162/git-town-action/pulls Let me know if you also want me to explore inserting the comment next to the first part of the comment |
|
I could also do that in a separate PR in order to get this fixed fast, and have minimal changesets |
|
@Eric162 Sorry about the late reply. Let's go with adding an additional anchor to the root list node. If we introduce an ending anchor users will still have their contents unexpectedly wiped from the PR description. You'll have to have to handle cases where stacks don't have this anchor yet and fall back to the current approach of looking for a list node immediately after the anchor. |
|
@tranhl updated the code, ready for review! |
tranhl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, really appreciate the work you're putting into getting this over the line! Just a few comments re: the approach that we need to work through 💪
|
@tranhl updates are submitted - ready for running the actions and a review? |
| '- main <!-- branch-stack -->', | ||
| ' - \\#1', | ||
| '', | ||
| '* [ ] this checklist', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question (non-blocking): I'm curious why the - would be replaced with * here. Is remark doing this? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tranhl yeah remark is adding it. My best guess was to somehow differentiate two lists, but not sure why that would even matter in markdown.
tranhl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will fix the CI/CD issue with EditorConfig checker on my end & test out the changes. If it looks good I'll ship a release. Thanks for contributing & making the action better!
|
This has been released in v1.1.0! 🎉 |
Description
In the action, when there is another list in the PR description, but the git-town action has not run, it will delete everything between the git-town comment and that list:
e.g.
This is because the code to replace the description is not greedy enough.
Stack