Issue #1038 - Include comment on pr close - #1123

The block of code i created on "pr.go" inside "func prClose" can go outside to a function "func prComment" or something like that, and this function can be reused and called by a single line on prClose (before closing the "PR"), or any place what need comments.
This function will allow to create a new command on PR Commands (ex: gh pr comment #).
This function can be called on anothers commands, like func prReopen to ask a user to add comment before reopening the "PR".

Hey @gabgodbb
(I am also a contributor like you. For official comments/suggestions, ask @mislav , @vilmibm etc )

I went through your PR:

1. Please fix the linting and build errors. (if you are using VSCode, install the Golang extension. It will show you errors wherever they are occurring, and may also auto-correct a few of them)
2. We are using a library called "survey" for User Input/Output, and not directly the golang standard way. You can refer to the code in the other files, for an example of how we are prompting for input.

And no, we don't have to make a test case for waiting for the user input.

Also, please confirm with the official team, for the exact command name, working and flow, that we are aiming for. (Its always better and recommended to wait for their official acknowledgement, before starting some new feature/enhancement)

Hey @gabgodbb
(I am also a contributor like you. For official comments/suggestions, ask @mislav , @vilmibm etc )

I went through your PR:

1. Please fix the linting and build errors. (if you are using VSCode, install the Golang extension. It will show you errors wherever they are occurring, and may also auto-correct a few of them)

2. We are using a library called "survey" for User Input/Output, and not directly the golang standard way. You can refer to the code in the other files, for an example of how we are prompting for input.

And no, we don't have to make a test case for waiting for the user input.

Also, please confirm with the official team, for the exact command name, working and flow, that we are aiming for. (Its always better and recommended to wait for their official acknowledgement, before starting some new feature/enhancement)

@AliabbasMerchant i installed Visual studio, i was coding on single text Editor.. and installed goLang extension.. the lint errors give on code on code i didnt changed. i got a fresh git clone, and the lint give a lot of that warnings, basically the same warnings... can you help me about this?
i had seen "survey" package, and it use nano.., but in CONTRIBUTTING.md it tell to avoid use something like that

@gabgodbb Yes, there are warnings generated by the linter in VSCode. Ignore them.
Just make sure that there are no errors. That should be enough, I think. And the CI tests here on your PR should pass.
We are using survey everywhere in CLI, so there is no issue in using it. You don't have to write the code and tests for handling user input. Please don't use the standard way. Your PR won't be accepted if you do that.
Also, I believe we are expecting --comment to be a flag in gh pr close, and not a separate command in itself.

thank you for the reply @AliabbasMerchant

I didnt use the survey because it use "nano".

In CONTRIBUTING.md, it say:

Please avoid:

• adding installation instructions specifically for your OS/package manager"

Someone explain for me, is possible to use go tools, like all lints check, on my local system? i am using Visual Studio with golang extension.

Thank you.

Nano is a text editor that survey uses for some input.
The docs state that you should not include installation instructions for CLI for your specific OS/Package Manager.
These both are completely unrelated, don't worry.

The whole of CLI uses survey for input/output. So, you do not need to worry about that.

Yes, you can definitely run the linter locally. Although, I dont know how exactly to do that.
This should provide some reference: https://github.com/cli/cli/blob/trunk/.github/workflows/lint.yml

@AliabbasMerchant
yes, thank you.... i am think this survey, and/or mine solution on "close" and "reopen' will break bash commands...
like: gh pr close 1;
git checkout;
gh pr close 2;
gh pr close 3...

on example, the pr close 2 and 3, will not to run, because it will be on #1 waiting for user input.... so i am think to create --comment on default... and create a --nocomment to dont ask for user input, and let bash commands continue without user interation...

@probablycorey i removed the "gr pr addcomment" function and put that 2 suggested changes on the same commit..

thank you for review

Thanks @gabgodbb, it looks like there are a few more issues.

1. Close and reopen should take the comment non-interactively, it would look like this gh pr close --comment "my comment" and gh pr reopen --comment "my comment". Having a prompt to accept the comment is something we might want to add in the future, but for now it should only take the comment via the --comment flag.

2. I'd remove the prComment function have prOpen and prClose call the api function directly, something like this would be in prOpen and prClose:

comment, err := cmd.Flags().GetString("comment")
if err != nil {
    return fmt.Errorf("could not parse comment flag: %w", err)
}
if comment != "" {
    err := api.AddPullRequestComment(apiClient, baseRepo, prNumber, comment)
    if err != nil {
        return fmt.Errorf("failed to create comment: %w", err)
    }
}

I also modified the api function a bit. I changed it to AddPullRequestComment and made it take a prNumber and a comment. This way seems like a more direct way of adding a comment.

Cool... Thank you for reviewing

Thanks @gabgodbb, it looks like there are a few more issues.

1. Close and reopen should take the comment non-interactively, it would look like this `gh pr close --comment "my comment"` and `gh pr reopen --comment "my comment"`. Having a prompt to accept the comment is something we might want to add in the future, but for now it should only take the comment via the `--comment` flag.

2. I'd remove the `prComment` function have `prOpen` and `prClose` call the api function directly, something like this would be in prOpen and prClose:

comment, err := cmd.Flags().GetString("comment")
if err != nil {
    return fmt.Errorf("could not parse comment flag: %w", err)
}
if comment != "" {
    err := api.AddPullRequestComment(apiClient, baseRepo, prNumber, comment)
    if err != nil {
        return fmt.Errorf("failed to create comment: %w", err)
    }
}

I also modified the api function a bit. I changed it to AddPullRequestComment and made it take a prNumber and a comment. This way seems like a more direct way of adding a comment.

I Believe is not a good practice sending comment via "parm" and we will get some issues if comment is sent by "--comment parm" while in text having quotes and/or double quotes , anothers special characters and some system variables.. like, PATH and password AND $USER, etc. "
Example:
if in the comment have the following sistuation above:

--comment "This code must pass on ""; go test ../...; "please run tests

The command "go test" will be EXECUTED, on system.. the danger of this if is a command to remove ( "rm" ) or print some variable or executing any code user is pasting or typing..

I Believe is not a good practice sending comment via "parm" and we will get some issues if comment is sent by "--comment parm" while in text having quotes and/or double quotes , anothers special characters and some system variables.. like, PATH and password AND $USER, etc.

@gabgodbb I see your point and I agree. However, when we enable adding comment via a person's text editor or from standard input, for example, we want to do that in the future and as a separate changeset from this one.

So right now, let's just support the optional pr close --comment '...' flag and nothing else. This will also be easier to implement. The user is responsible for encoding the comment properly in their shell.

@probablycorey i make a new resolution about this, for learning, and it is more simple... just using --coment args, can i close this pr and open a new fresh one?

@gabgodbb For sure, go right ahead!

In the future, you can also "reset" your PR by force-pushing entirely different commits to its head branch. That's how I undo my changes if it turns out I have been heading in the wrong direction.

Creating a new PR with --comment option

@gabgodbb For sure, go right ahead!

In the future, you can also "reset" your PR by force-pushing entirely different commits to its head branch. That's how I undo my changes if it turns out I have been heading in the wrong direction.

done pr #1395, can you check if i deleted my old branch, "Including_pr_Close"

@gabgodbb For sure, go right ahead!

In the future, you can also "reset" your PR by force-pushing entirely different commits to its head branch. That's how I undo my changes if it turns out I have been heading in the wrong direction.

@mislav done, check PR #1395