Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flexible exec #42

Closed
wants to merge 9 commits into from
Closed

Flexible exec #42

wants to merge 9 commits into from

Conversation

tony
Copy link
Contributor

@tony tony commented May 5, 2016

Fixes #39

  • decouples commands from base.Get, base.Init, base.Update into GetCmd, InitCmd, UpdateCmd to return standard library's *exec.Cmd. This gives downstream users the flexibility to access repo commands as they choose.
  • Current behavior of base.Get, base.Init and base.Update are method is preserved.
  • Two new methods, base.runCmd and base.RunCmdFromDir accept standard library *exec.Cmd as arguments. base.run and base.RunFromDir behave the same to preserve compatibility downstream.
  • A lot of redundant code has around parent directories being being created has been moved to EnsureParentDir(). This reduces the complexity inside of Init and Get methods.

@mattfarina
Copy link
Member

@tony I have some questions.

  1. Why are the commands exposed separate from running them? Can you share an example of how you will use this? I'm looking for use cases to justify the added complexity to the interface.
  2. I noticed the addition on Git of Fetch methods that are public. This doesn't fit with the consistent interface. Why do you think they should exist as public methods?
  3. If the run function is both internal and deprecated what's stopping it from being removed all together?

@mattfarina
Copy link
Member

One more thing. Yesterday I merged in some new methods. Those would need to be converted as well. But, first the questions.

@tony
Copy link
Contributor Author

tony commented May 6, 2016

  1. Making the standard library primitives exec.Cmd accessible on the object is a good long term choice. Though, I see the convenience of just running the command has to some. This is a library / tool, in its current form, the only publicly accessible way to clone/checkout a repo is .Get, forcing the user to run the command immediately, while not giving them an option to access the standard library exec.Cmd themselves.

    My example, 1) I don't want to run the command immediately 2) I also need want to be able to multiplex the live output that command does. Making the standard libraries command available alleviates both problems by allowing me to .Start the command and tweak the stdout/stderr. For a WIP example: https://github.com/tony/vcsync/blob/master/main.go#L33

    I'm totally new to buffers/channels/goroutines and hooking into the "buffered" stdout/stderr is still jarring to me. And I've been playing with a similar idea for >2 years.

  2. FetchCmd and FetchError: Git's .Update does a fetch and a pull. I'm decoupling into commands for the reason above. Fetch has a special error handler for detached states. In my situation, I'd be composing a command via r.FetchCmd() and checking for errors via r.FetchError(err).

    I am open to other opinions if you have any. I don't want to duplicate behavior in this library downstream just because I need a little bit of granularity.

  3. Because I PR'd Make RunFromDir exportable #25. I think it'd feel out of order to have a .run accepting exec.Cmd, but then a .RunFromDir which still accepts the old argument form. I don't want to break compatibility with .RunFromDir's signature. Though, its only been a few weeks. IMO I think its better to throw around the standard library exec.Cmd rather than cmd string, args ...string.

    What do you think if we just made RunFromDir and .run accept exec.Cmd and remove the .RunCmdFromDir and .runCmd?

@tony
Copy link
Contributor Author

tony commented May 6, 2016

Spelling / corrections above - a tad sleep deprived atm.

@mattfarina
Copy link
Member

@tony thanks for sharing your thoughts. I understand correcting things due to writing while needing sleep.

Have you seen the talk by Rob Pike on Simplicity is Complicated. It's about Go and shares a philosophy that's baked into Go.

Making things simple and hiding the complexity is hard but it's a goal to strive for. This same philosophy is in the vcs package provided by the Go team.

This change greatly complicates things for end users. It starts to showcase the complexity rather than making it simple to work across VCS. I'm fully aware that hiding the differences in VCS makes accessing their unique features difficult or impossible. There are times custom code is needed to make that happen for unique situations.

That said, I'm open to solving problems and making things better.

From your comments I see a bit about how you want to build your application but I'm not entirely sure why you want to do it that way? Why are you trying to go as close to unbuffered as possible? How are you using the difference between Stderr and Stdout? In the code snippet you shared you are combining them.

What I'm trying to do is look for an alternative that hides complexity while meeting needs.

For example, there is a logging mechanism. Currently it captures the output once complete. I wonder about piping the output to the log as an option. Or something similar.

@mattfarina
Copy link
Member

I keep coming back to this one and have decided not to enable flexible exec. If someone wants to fork this package to do so or create their own I'm comfortable with that.

Sorry it's taken me to long to decide on this one.

@mattfarina mattfarina closed this Jan 18, 2017
@tony tony deleted the flexible-exec branch January 18, 2017 18:48
@dmitshur dmitshur mentioned this pull request Jan 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants