search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

[question] re: ... "two hard things" ... "naming things" #39

Closed andreineculau closed 8 years ago

andreineculau commented 8 years ago

Disclaimer: this PR's goal is not to be merged, nor to be negative towards the current status, but to feed my curiousity on a few naming decisions

I was intrigued when reading README.md and docs/tutorial.md e.g.

Kindly have a look at the PR's diff and add comments inline, if you will.

In any case, thanks for the nice effort with putting up git-rev..., I mean git-appraise ;)

hazbo commented 8 years ago

Hey @andreineculau,

You raise some interesting points here. I'm sure @ojarjur will be able to respond in more detail but I figured I'd weigh in here.

to see no correlation, nor explanation, as to why this is called git-appraise, although we talk about reviews

Your right, in terms of the docs and the code itself the term review is used, rather than appraise.

git-appraise is simply just the name of the project and also the git command. However I find the words 'review' and 'appraise', under the given context, somewhat synonymous given that to appraise, is to "assess the value or quality of", which I find works quite nicely with this project as that is literally the reason why someone would request a code review. Personally I prefer appraise over review as both the name of the project and the git command - that's just my opinion though ;)

submitting vs merging/rebasing

I'm not sure whether or not by this you mean; "why would I use git appraise submit 8cb887077783 as appose to git merge?", or "why isn't it git appraise merge 8cb887077783 instead of submit?". Either way they're both perfectly valid questions, so I'll answer both.

1) submit is one subcommand that deals with both closing the code review (changing it from 'accepted'), and the way in which the code is actually merged. So with this subcommand you may use --merge or --rebase or --fast-forward, or you may configure this globally within the scope of your project. By default it will use fast-forward. If you're using git-appraise in any meaningful way, the submit command seems to make the most sense.

2) Git already has merge and rebase subcommands. The focus there is on merging the code. The focus with git-appraise is to close (submit) the review along with the code. So rather than having 3 subcommands to submit the review we just use submit and optionally allow the flags at stated above.

These are just my opinions on two of the points you brought up. I hope this provides some insight. :)

andreineculau commented 8 years ago

thanks @hazbo . I will sprinkle the diff with the questions, for you and @ojarjur, as it provides context e.g. I meant "why git appraise submit --merge instead of git review merge?"

hazbo commented 8 years ago

@ojarjur it'd be interesting to hear your take on this PR.

ojarjur commented 8 years ago

@andreineculau, thanks for the feedback and questions.

@hazbo, thanks for weighing in.

To begin with, sorry it took me a while to respond; this was a long weekend for me, so I wasn't at my computer.

There's a lot of subtle issues around naming and I need to wait until next week before I can give all these questions the amount of thought they deserve. I'll come back with more in depth responses next week, but I did want to address the 'git-review' issue now:

I originally did want to name the tool that, but there were multiple other tools already called that, so that name would not work. Using 'git-appraise' instead removes any ambiguity.

I also thought about calling the reviews 'appraisals' in the docs, but since 'review' is already standard terminology, that would be confusing.

In essence, for the docs, widely used terms are better, but for the tool's name uniqueness is better.

andreineculau commented 8 years ago

@ojarjur once again, take it easy :)

but since 'review' is already standard terminology, that would be confusing.

FWIW, while it is standard terminology, it is not a confined concept. Like "fruit" is standardized terminology, but both apples and pears fit the box. All in all, I'm more interested in the other suggestions, as git review can be a local alias.

there were multiple other tools already called that

care to name/link them? next week! :) Thanks

hazbo commented 8 years ago

Hi @andreineculau

I just happened to come across this while reading about Gerrit, it's the code review system that the Go language itself uses. Apparently this relies on a program called git-review.

Interestingly, while then reading about git-review, I saw this:

The upstream project is led by OpenStack. Is not to be confused with the unrelated Facebook project.

Which took me to this. It appears this project is no longer worked on / used. Just figured I'd update this thread upon seeing this. :)

ojarjur commented 8 years ago

Thanks again for all of the feedback @andreineculau. I've gone through (I think) all of the inline comments and either created an issue to track the corresponding changes, or added a comment about why I don't think something will work.

If I missed anything, then please call that out.

As for the other tools named "git-review", Harry found the biggest one, but there is also a tool called "git-codereview", and hundreds of smaller projects on github that use the name "git-review".

andreineculau commented 8 years ago

Thanks for all the effort and time