search

kr / hk

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

Promote app parameter #57

Closed tt closed 10 years ago

tt commented 10 years ago

I made a pull request for #56. Let's keep the discussion about the feature in that one, but please comment on my code here.

kr commented 10 years ago

This PR is beautiful. I just have a couple of comments.

kr commented 10 years ago

Good call to remove caching the app name. Much better now, thanks!

bgentry commented 10 years ago

Oh, I only just saw that this existed. Appears to be still good to merge after v3 changes. @kr still waiting for the requested changes though, right?

kr commented 10 years ago

Yeah, LGTM after fixing shadowed flagApp.

tt commented 10 years ago

I completely missed the comments, but I made the fix.

bgentry commented 10 years ago

@kr oh, I wasn't sure on this.. for this sort of PR with nice, small, simple commits, did you still want me to squash them before merging, or is it ok to just pull these in on top of master?

kr commented 10 years ago

My usual rule of thumb is, if each commit makes sense by itself, then it's fine/preferable to leave them separate. (And if you ever disagree with my application of that rule, feel free to call me out on it!)

So in this case I think it's fine to leave them all separate. Each one pretty much stands alone.

bgentry commented 10 years ago

Rebased and merged as 9f90a5fa65004c128e4cc761f1760c074cb79a58 to a011c7183a2b9e90918b150d61b75e75ca7d4099

Thanks @tt!

tt commented 10 years ago

You're welcome.

I think you should reconsider the no-merges policy (if there is one). I definitely prefer rebasing topic branches to keep them in sync, but I think it's best bringing them into the history by a merge. This marks that a branch existed and it's a bit more honest: the development did happen in parallel with other things. I usually commit so that everything works in each commit, but rebasing may break that, which in turn will make it impossible to use a tool like git-bisect.

kr commented 10 years ago

Yeah, I understand the worry about breaking git-bisect, but you can also avoid that worry by making each PR a single commit. :)

The git history isn't for recording the development process, it's for facilitating concurrent development on a shared codebase, by making it easier to do things like rebase your work in progress. Extra merge commits don't make that any easier; they just add noise.

tt commented 10 years ago

You should probably just use --squash when merging them so you preserve readability until then. I think the argue against is that it's a lot less obvious if you have to revert a change, because you in that cause would appreciate the readability too.

Merge commits serve a purpose when doing distributed development; they denote that both parents are still in the history. If you alter the history before merging it, one or more peers will not be aware that the branch merged without explicitly reading the history.

kr commented 10 years ago

Yes, once someone has based work on top of a commit, it's disruptive to rewrite that part of the history. But for even quite large projects it's usually easier for everyone to simply not base any new work on top of branches that haven't been merged yet.