Add support for last comment edit #6384
Conversation
pkg/cmd/pr/shared/commentable.go
Outdated
| return err | ||
| url := "" | ||
| if opts.EditLast { | ||
| username, err := api.CurrentLoginName(apiClient, repo.RepoHost()) |
There was a problem hiding this comment.
api.CurrentLoginName is retrieved as "github-actions[bot]" when executed with GitHub Actions GITHUB_TOKEN: ${{ github.token }}, but Comment.Author.Login will be retrieved under a different name, "github-actions", so there is no match.
Should I trim "[bot]" if it is "github-actions[bot]"?
There was a problem hiding this comment.
When creating a PAT, --edit-last works fine, but when using github.token in CI, there is a problem with the username not matching and not getting the last comment.
I don't know why the names don't match, should I create an issue?
There was a problem hiding this comment.
@seachicken What is the error you are receiving? Is it showing the incorrect user or not retrieving any user at all?
There was a problem hiding this comment.
I get an incorrect username.
api.CurrentLoginName returns "github-actions[bot]"
Comment.Author.Login returns "github-actions"
vilmibm
left a comment
There was a problem hiding this comment.
Nice start! I'd like to see the comment's body populated in the editor, however, otherwise it's not really an edit operation. I'd also like to see more robust tests -- in other words, test cases that actually cover this new behavior.
|
Now works in combination with other commands. |
There was a problem hiding this comment.
@seachicken Thanks for working on this. There are some changes I would like to see in Commentable that are mostly about keeping the code clear and maintaining the original intent of Commentable. I added the comments inline so please let me know if you have any questions regarding them.
samcoe
left a comment
There was a problem hiding this comment.
@seachicken Thanks for making the requested changes! I pushed a small commit adding a bit of polish 💅. This looks ready to ship.
Only question I have is about the name of the flag. --edit-last was agreed upon in the issue but with the upsert functionality do we think we should reevaluate the flag name? @vilmibm @mislav what are your thoughts?
|
As a person who first floated the idea of "upsert" functionality in the original issue, I don't think that this functionality needs or should be part of |
|
I went ahead and made the change to remove the upsert functionality and also I fixed the issue with comparing username strings for bots by switching to use the |
Changes requested have been addressed.
pkg/cmd/pr/shared/commentable.go
Outdated
|
|
||
| comments := commentable.CommentsForUser(username) | ||
| if len(comments) == 0 { | ||
| return fmt.Errorf("no comments created by %s found", username) |
There was a problem hiding this comment.
The error when there is no comment makes it hard to use it from the bot. I and the original issue would like an option to use it from the bot.
If --edit-last is not a good name, it would be more convenient to use an option name such as --upsert to avoid the error.
There was a problem hiding this comment.
@seachiken The team decided that the upsert functionality added complexity and confusion due to the naming of the flag. We also decided that it doesn't need to be built in since it is easy enough to recreate it by first doing issue comment --edit-last and if that fails then doing issue comment.
| var comments []Comment | ||
| for _, c := range cs.Nodes { | ||
| if c.Author.Login == username { | ||
| if c.ViewerDidAuthor { |
There was a problem hiding this comment.
I didn't know that. Great!
Fixes #3613
Add
--edit-lastoption toissue/pr comment.This option edits the last comment that you are the author of. If the comment does not exist, a new one is added.
This is mainly useful when editing comments by the bot.