Open braughtg opened 4 months ago
Running ./squashMergePR.bash
gives me a GraphQL error when trying to fetch PR details. I see the same error if I use the --pr-number
flag.
If I continue, the type and scope I enter are not inserted into the proposed commit message:
If I then (E) the commit message to add a type and scope I get the following error message:
If I specify the --type
and --scope
on the command line, I am being prompted for them anyway:
Running
./squashMergePR.bash
gives me a GraphQL error when trying to fetch PR details. I see the same error if I use the--pr-number
flag.
If I continue, the type and scope I enter are not inserted into the proposed commit message:
If I then (E) the commit message to add a type and scope I get the following error message:
If I specify the
--type
and--scope
on the command line, I am being prompted for them anyway:
Few in progress notes, I got the script to run after messing around with the regex and gh
Still working on finishing up cli flags and adding PR numbers but I just want to set the expectations for a review request by tomorrow.
One thing I am struggling with though is the decision of adding existing details from the PR.
if [ -z "$PR_TITLE" ] && [ -z "$PR_BODY" ] && [ -n "$PR_NUMBER" ]; then
echo "Fetching PR #$PR_NUMBER details..."
PR_DETAILS=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json title,body)
PR_TITLE=$(echo "$PR_DETAILS" | jq -r '.title')
PR_BODY=$(echo "$PR_DETAILS" | jq -r '.body')
fi
It can pull in PR Details, do we want to modify the pr title and body in nano to our specs or do we just want to just open the PR in the web and then copy paste or type up our own sections from scratch without retrieval if that makes sense?
Sounds like progress. I’m away until Monday but can have a look at it early next week.
On Mar 5, 2024, at 22:06, Ty Chermsirivatana @.***> wrote:
Few in progress notes, I got the script to run after messing around with the regex and gh
image.png (view on web)https://github.com/FarmData2/squashMergeTest/assets/28925758/7597a2e6-6a94-4120-8461-1cf85589e0da
Still working on finishing up cli flags and adding PR numbers but I just want to set the expectations for a review request by tomorrow.
— Reply to this email directly, view it on GitHubhttps://github.com/FarmData2/squashMergeTest/issues/2#issuecomment-1980096068, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEBNHMUZKUYZ2OWQZVWEJLTYW2P63AVCNFSM6AAAAABEDJJ3H6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBQGA4TMMBWHA. You are receiving this because you authored the thread.Message ID: @.***>
It can pull in PR Details, do we want to modify the pr title and body in nano to our specs or do we just want to just open the PR in the web and then copy paste or type up our own sections from scratch without retrieval if that makes sense?
I would like the script to be as self contained as possible. So that when it is time to squash merge a PR just running the script is sufficient. So, I think we should pull in the pr info and then format everything as close as we can to what we want for the conventional commit and let them edit in nano
.
Looks like jq
is a dependency that will need to be added to the dev environment when this is merged.
@WarpWing This seems to making good progress. I am still seeing some unexpected behavior though.
y/n
or yes/no
.@WarpWing This seems to making good progress. I am still seeing some unexpected behavior though.
- For yes/no prompts it would be nice to be able to use
y/n
oryes/no
.- In the second run
- It properly extracts the title and body but then seems to build a conventional commit using them even if it doesn't find a valid type and scope (e.g. 'create newfile(create newfile): ...`
- Then it tried to merge PR 5 instead of PR 8 which is the one I indicated on the command line.
Ahh, I think I know why those are happening, I'll fix it and flag it by this afternoon.
@WarpWing This seems to making good progress. I am still seeing some unexpected behavior though.
* For yes/no prompts it would be nice to be able to use `y/n` or `yes/no`. * In the second run * It properly extracts the title and body but then seems to build a conventional commit using them even if it doesn't find a valid type and scope (e.g. 'create newfile(create newfile): ...` * Then it tried to merge PR 5 instead of PR 8 which is the one I indicated on the command line.
I've made some modification to fix the weird created commit and fixed the #5 ghost commit.
However, this part here:
I believe is part of gh cli.
I do want to fix some spacing consistencies in prompting however.(newline vs sameline on some question prompts)
Changelog
@braughtg In other words, I've made sure to clean house and check for errors. I'm not sure how to make jq
a part of the dev enviroment. I can look into that and make the appropriate merges (I would imagine this script would be useful on DickinsonCollege/FarmData2
as well as the new repository, I'll make sure to PR the script on both repos). I'll set up another "joke" PR for testing and am currently awaiting any feedback.
Also, just doing morning review, I'll write in behavior to accomodate both (y/n) and (yes/no) inputs by this afternoon, just need you sign off on everything else.
I've further found some errors or unexpected behavior give if the PR number is not valid or found OR if it still computes an UNKNOWN PR state (which implies that merge-ability is still being calculated).
I'll take a look at creating some conditions to check for valid PR overall and remove the UNKNOWN check.
@braughtg I believe I've fixed the behavior. I would estimate it's 95% ready, I'm still looking on some of the script logic and fixing it where it makes sense. I think I need to work on adding default handling for blank cli parameters (which we could technically leave alone assuming people with this script would know what they're doing but it's more efficient to handle all user behavior). Outside of that, we should be all set to go. Please let me know if you need me to do any touch ups and feel free to test via #14.
@WarpWing Nice changes so far and I think you have all of the necessary techniques worked out. It successfully squash merged #14. But there are a few things that need to be addressed:
The command line args for type and scope should be validated. When I merged #14 i used only --type
and set it to junk
which is not a valid type for the project but the PR was merged with type junk
.
elementInArray
function that should be helpful for this.The parsing of information from the current PR doesn't seem to be working quite right.
When I continued through on the above and answered the prompts the PR was merged. But the commit message for the squash merge had the commit message and body a little mixed up.
BREAKING CHANGE
message is in the message footer. But when the PR is merged the body of the message appears after the footer.
feat(dev): comments on joke
Commented that the joke was funny!
BREAKING CHANGE: pretends that this breaks something.
- I think the `pr-number` command line flag should be shortened to `--pr` for convenience.
@WarpWing One way to proceed would be to test something like the following:
Then check at each point that the user is prompted only for missing parts (e.g. no type), invalid parts (e.g. type of junk
) or parts or for parts that conflict with provided command line arguments (e.g. --type
is provided and there is a type in the PR title).
If I was doing this I would probably create all of the test PRs and then comment out the last part of the script that actually does the merge until I know it works on all of them. Then put back in the actual merge. Then if something doesn't work you can revert the PR and try again. Could be a little tricky avoiding merge conflicts, so I might also use a different test file for each PR.
We can work on the script in this repo.
The best way to test this script is to:
testfile.txt
fileI'll place comments below on the experiences as I test the script.