Testing the `squashMerge.bash` script

[braughtg]

We can work on the script in this repo.

The best way to test this script is to:

• Modify the the testfile.txt file
• Create a PR for that change.
• Run the script to try to squash merge that PR.

I'll place comments below on the experiences as I test the script.

[braughtg]

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:

[WarpWing]

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?

[braughtg]

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 GitHub<#2 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEBNHMUZKUYZ2OWQZVWEJLTYW2P63AVCNFSM6AAAAABEDJJ3H6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBQGA4TMMBWHA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[braughtg]

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.

[braughtg]

Looks like jq is a dependency that will need to be added to the dev environment when this is merged.

[braughtg]

@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.

[WarpWing]

@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.

Ahh, I think I know why those are happening, I'll fix it and flag it by this afternoon.

[WarpWing]

@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.

[WarpWing]

I do want to fix some spacing consistencies in prompting however.(newline vs sameline on some question prompts)

[WarpWing]

Changelog

• Added checks to make sure type and scope inputs are valid
• Removed any debug statements
• Added checks to make sure if there is a breaking change footer and the users says no to ensure that a footer has been found and gives options to edit it
• fixed any previous concerns raised
• Added more clarity such as commit author, clean working tree, clean status,draft checks and merge status (Open,Closed,Merged) etc.

@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.

[WarpWing]

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.

[WarpWing]

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.

[WarpWing]

@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.

[braughtg]

@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 Update testfile.txt #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.
  

  • You have the elementInArray function that should be helpful for this.

• The parsing of information from the current PR doesn't seem to be working quite right.

  • Here I created a PR with a valid conventional commit (see feat(docs): comments on joke #15) but the type and scope were not recognized.
    
    • The type and scope should be parsed out of the PR title if they are there.
  • Here the PR also includes the BREAKING CHANGE footer but wasn't picked up and I was prompted for whether the change was breaking or not anyway.
    

• 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.
  

  • The body "Commented that the joke was funny!" seems to have been added into the title and should only appear in the body.
  • The proposed message above shows that the BREAKING CHANGE message is in the message footer. But when the PR is merged the body of the message appears after the footer.
    
  • For PR feat(docs): comments on joke #15 I would have expected:
    • To not be prompted for type or scope since valid type and scope were in the PR title.
    • The commit message to be:

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.

[braughtg]

@WarpWing One way to proceed would be to test something like the following:

• Create a PR like feat(docs): comments on joke #15 that has all the valid pieces type, scope, title, body, BREAKING CHANGE and be sure that the script parses each one of them correctly.
• Create a number of PRs like feat(docs): comments on joke #15 but:
  • One with invalid type
  • One with invalid scope
  • One with no type
  • One with no scope
  • One with no title after the type(scope)
  • One with no body
  • etc...

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.