kr / hk

Fast Heroku client
https://hk.heroku.com/
77 stars 6 forks source link

Specify app by git remote. #46

Closed danp closed 11 years ago

danp commented 11 years ago

This adds the ability to specify app by arbitrary git remote instead of just heroku. It's by far the feature I am currently missing most from the full client.

kr commented 11 years ago

How do you feel about this being a separate flag vs being accepted in -a along with the actual app name? I think of it like a git commitish, which can be a tag, branch, or hash (or even a complex thing like master~4^2~3). Similarly, an hk "appish" could be either a remote or an app name.

danp commented 11 years ago

That could be good. Thinking through preference order...how does this sound?

  1. if git remote -v works AND if appish names a git remote AND an app name can be extracted from that URL, use the extracted app name
  2. consider appish to be an app name

Any concern about remote names and real app names colliding? I don't see this being a problem for me at least as I end up with remotes like production, staging, dan, etc., where these are usually suffixes of the base app name, eg foobar-production. But should there be a collision I could see it being confusing.

kr commented 11 years ago

I'm not too worried about collisions. If someone has a git remote for one app with the same name as another app, well that's just asking for trouble. Falls squarely under the category of "don't do that".

We can also take care to provide a good error message and docs so that people have a good chance of knowing what's happening.

kr commented 11 years ago

And I agree completely with the preference order you gave, though I think the first thing, git remote -v, is redundant with looking up the remote using git config. So I'd give the rules as:

  1. if git config remote.appish.url indicates success and its output is a well-formed heroku git url, use the extracted app name
  2. else consider appish to be an app name
kr commented 11 years ago

Hmm it's not totally clear from the github ui that this was merged, but anyway it is merged! Thanks!