profclems / glab

The GitLab CLI tool. Archived: now officially adopted by GitLab as the official CLI tool and maintained at https://gitlab.com/gitlab-org/cli. See https://github.com/profclems/glab/issues/983
https://glab.readthedocs.io/
MIT License
2.08k stars 163 forks source link

Add feature to review merge-request #679

Open l0nax opened 3 years ago

l0nax commented 3 years ago

Describe the feature or problem you'd like to solve

As described in #80, which has been closed and labeled as resolved:wontfix without a reason, a sub-command, probably mr review <id>, to review a merge-request would be a really nice feature.

Propose a Solution

Future discussion needed, on how the TUI would look like. But only if this type of feature is considered worthy to be implemented.

Additional context


Side note: I discovered this project a few days ago and for a while, several months, I was considering writing such an application (with this code review feature). This project is amazing. Thanks!

profclems commented 3 years ago

Hi @l0nax thanks for opening this issue and sorry for the late response, I went off for a while.

I also think this feature will be a great improvement for glab and I opened the issue #80 to work on that feature. You're right, I closed it and labeled it as resolved:wontfix because I couldn't find the time to work on the feature. Working on it will be a bit time consuming and since I am not sure when I can work on that, so I decided to close it so that users will be kept waiting for this feature which may never happen. Sometimes it's better to say 'no' to a feature than keep it hanging. I'm glad you reopened this issue... I will keep it open and put it in my backlog.

I will definately find time to work on it this time round.

profclems commented 3 years ago

I am still contemplating on how the TUI should look like for this feature. This is open for suggestions.

cc: @zemzale @l0nax @maxice8

b4nst commented 3 years ago

Throwing my 2 cents here. Something like opening an editor file by file with diff. Everything you would add here would be treated as a GitLab comment, maybe by delimiting each comment between 2 tags (~?). And I would say the easier would be to use the same format as GitLab is using in the GUI. Example for #447 that would translate to something like:

@@ -29,5 +55,7 @@ func NewCmdRepo(f *cmdutils.Factory) *cobra.Command {
    repoCmd.AddCommand(repoCmdFork.NewCmdFork(f, nil))
    repoCmd.AddCommand(repoCmdSearch.NewCmdSearch(f))

+   repoCmd.Flags().BoolVarP(&opts.OpenInBrowser, "web", "w", false, "Open repo in a browser. Uses default browser or browser specified in BROWSER variable")
~
can't it be a `BoolVar` instead and instead of creating a struct one just does `repoCmd.Flags().Changed("web")` ?
~
    return repoCmd
}

The hard part would be dealing with existing comment maybe.


Whatever we decide to go for, I'm really interested in such feature and I can help developing it.

zemzale commented 3 years ago

My first thought for this was basically replicating the GitLab GUI into a TUI, but this kind of option that you can review the MR in your own editor would be pretty cool,

I just don't know how would things like responding to comments or commenting on multiple lines at the same time work.

Also from a UX perspective, with a TUI it's possible to just add some help window were the user can see what keybind does what, where opening file in editor would force the user to learn some special syntax before even using the review feature.

b4nst commented 3 years ago

I understand your point. I have a different opinion on the TUI, probably because we all come here for different reasons. I'm not really found of the TUI parts (like ci view for example) because to me it's just another GUI. I think maybe providing the 2 solutions (a TUI and an in-editor integration) would be cool.

Regarding the UX you're totally right but that could be offset with auto-completion mechanisms (not saying this would be easy).

I also agree that this is not a simple flow. A lot of details would have to be discussed.

zemzale commented 3 years ago

I agree on the part that a TUI is just another GUI, but I like that I don't have to switch between a browser and my terminal.

I like the idea of experimenting with a TUI and a in editor reviewing at the same time.

profclems commented 3 years ago

The TUI with an in-editor reviewing is a nice idea and I totally agree with it. That will definitely come with inline commenting, which would require the user to learn special syntax as @zemzale rightly said.

This isn't going to be an easy task as we all know it and it will take much time and dedication to finish it up. Either way, I'm ready to experiment with it.

The hard part comes from dealing with existing comments, replies to comments and multiple comments on a single line.

Another thing to decide on is to either display the changes by commits or by changed files.

clemsbot commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. We haven't had the time to address it yet, but we want to keep it open. This message is just a reminder for us to help triage issues.