Closed grantseltzer closed 8 years ago
Do not merge, most recent commit was just to save my work. I'll rebase it when I finish it tomorrow. (Good progress though!)
@diwakergupta @waits Let me know how this looks. here's a demonstration of it in action.
@GrantSeltzer thanks for the PR. One high-level comment: rather than make one off fixes for each command, it would be nice if we had a holistic plan for the output in general. Basically create a foundation to add more features in future (e.g. colorized output). One suggestion would be to use something like https://github.com/nsf/termbox-go. Thoughts?
@diwakergupta That's a great idea. We could have a display service (possibly using termbox-go). That way all of the commands could have a standardized way of sending output back to the user.
Also, what do you think of having an interactive mode for dbxcli? Make it as if the user is ssh'ing into their dropbox but the back end is still the public rest api.
@diwakergupta This PR can be closed, but I would love to discuss ideas for output. I put the code from this pr into this repository which can eventually be moved into dbxcli if you'd be interested. Feel free to comment on that, or send me an email to discuss.
@GrantSeltzer yeah having a simple way to allow commands to create well formatted (in future possibly colorized) output makes sense to me. Whether we create a "display service" is an implementation detail.
Interactive mode has come up in the past, so I'm open to exploring that but would prefer to hold off until there's very clear use case/demand for that, because it will add a bunch of complexity.
Thanks for pulling out the display library. I see that you've removed the os.Exec
call, which is great! Let me look at the current PR again to see how best to move forward.
@diwakergupta If you're happy with the functionality here I can change the PR so that it uses the library I separated out.
I would like to see the library be hosted inside the dbxcli repo, what do you think?
@GrantSeltzer let's merge this PR as-is. I will then vendor in the golumns lib and we can refactor this. Unless you want to vendor it in upfront, that's fine by me as well. Is that what you had in mind?
@diwakergupta I was thinking just vendoring it in to begin with. What is the motivation for merging it as is now if we plan on completely changing it right after?
@GrantSeltzer sure, go for it. Please use govendor
to vendor it in.
@diwakergupta You got it, will do that tonight.
I just updated dependencies so you should rebate before adding to vendor.
On Fri, Jul 15, 2016, 8:36 AM Grant Seltzer Richman < notifications@github.com> wrote:
@diwakergupta https://github.com/diwakergupta You got it, will do that tonight.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dropbox/dbxcli/pull/27#issuecomment-232986739, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA-dnVocAhkrKTpJrqtunEVXBSCxtiZks5qV6kFgaJpZM4JGgsx .
@diwakergupta I wish that I had read that last comment earlier :P
It appears that I've created a whole lot of commit spam in my effort to solve the conflict and then rebase. I'm pretty new to contributing to OSS/rebasing, would you mind helping me out, not sure how to fix the mess.
@GrantSeltzer never mind, just saw your message. Let me try and fix the commit up. Thanks for your patience!
Thank you for yours!
On Jul 16, 2016 1:13 PM, "Diwaker Gupta" notifications@github.com wrote:
@GrantSeltzer https://github.com/GrantSeltzer never mind, just saw your message. Let me try and fix the commit up. Thanks for your patience!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dropbox/dbxcli/pull/27#issuecomment-233140656, or mute the thread https://github.com/notifications/unsubscribe-auth/AKWrhSXzsAr7udGAig6QFZmN5KfybcM0ks5qWRFUgaJpZM4JGgsx .
Hmm @GrantSeltzer actually this commit doesn't work as is. Specifically, it breaks ls -l
(long listing). I might not get around to fixing it until later tonight.
Ah, looking into this now @diwakergupta
Also, if it would make your life easier I can just open up a new PR and i'll read through a git book I just got on how to properly rebase in the future :) Your call.
@GrantSeltzer at this point rebase is not the blocker. For now I'm going to default to the current behavior for long listing and only use Golumns for short listing. It'll be simpler for me to push directly rather than update this PR. That OK with you? I'll include original attribution in the commit msg :)
Actually just fixed up the author in my commit, so we should be good on attribution. Closing this PR, thanks!
@diwakergupta I actually just fixed it, was about to push :D
The long option, not the rebasing :P
My fix was the same as yours, no worries
@diwakergupta Thanks for the merge! I will be better with git in the future :+1:
Addresses Issue #17
Before and After
Signed-off-by: Grantseltzer grantseltzer@gmail.com