search

google / git-appraise

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

git-appraise without subcommand will list reviews #54

Closed hazbo closed 8 years ago

hazbo commented 8 years ago

As stated in #42, this will now show open reviews and not the usage text. I'm still a little unsure about this, because the list command is obviously still there and will work with flags, -a and --json for example.

Should this (non-subcommand version) allow the same, and if so, do we even need the list subcommand?

/cc @jishi9

ojarjur commented 8 years ago

Thanks for picking this up.

I'll admit I'm also a little unsure of what the right behavior should be. I'm currently leaning toward the opinion that we can't really win, so we have to go with the least-bad option.

I think that we should still keep the list subcommand, and that git appraise should not support all of the flags that git appraise list supports. The reason I lean towards that is because it:

  1. Will make the default behavior applicable more often (i.e. I want to list the open reviews very frequently, but very rarely want to see the help text).
  2. Will save us from having to make the default help text very complicated (i.e. the list flags only have to be shown in the output of git appraise help list).

That corresponds with the change you have right now, so I agree with your current proposal.

The downside of this approach is that we now have two ways to list the open reviews (git appraise and git appraise list). However, the same can be said for listing branches (git branch and git branch --list), so I guess we are at least being somewhat consistent with the git interface.

We should probably also update the default help message so that it explains that we list open reviews by default.

hazbo commented 8 years ago

I think this is starting to shape up a bit now. Although this may not be 100% consistent with how git branch / tag works, I don't think this is a case of not being able to win. I think that this is intuitive and straight forward to use. By keeping the list subcommand a user can specify flags to be more specific if they so desire.

Listing reviews without the subcommand is a nice touch. It's probably going to be used more than the list subcommand now anyway.

ojarjur commented 8 years ago

we can't really win, so we have to go with the least-bad option.

This was a poor choice of wording on my part. Sorry about that.

This change isn't a 'least-bad' option; it's a good option. What I was trying to get at is that there is no perfect solution, because we either have inconsistencies with git branch or we have multiple ways to do the same thing.

Regardless, this looks good to me now.

Thanks again for taking this on and getting it fixed!