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

Inline command write methods #41

Closed
wants to merge 4 commits into from

Conversation

robd
Copy link
Contributor

@robd robd commented Jul 7, 2015

I had another look through #38 and spotted a possibility for a bit of simplification in the ConsoleFormatter. I think this is an improvement, and all the tests pass.

One thing I wondered about is deferring the command = decorate(command) lines until after the return if debug?(command) lines, but the decorate method has some side effects in Context.decorate_command. I was wondering if we could make those side effects more obvious in the ConsoleFormatter.

The other thing is, I think these side effects only ever occur when decorate is called from the log_command_start method. Therefore, I was wondering if we should just have a new explicit method on context and call that from log_command_start. This would make context.decorate_command side effect free and I think it would make the code easier to reason about.

If you think this is worth investigating, I will work on it a bit and see what I can come up with.

robd added 4 commits July 7, 2015 12:31
Unify the command decoration for the different SSHKit versions. Now both the code path for old SSHKit versions ( via write) and the master (via log_command_*) do not decorate the command separately.
Call the new log_command_* methods from the write method
Inline the single usages of the write_command_start, write_command_output_line and write_command_exit methods
@mattbrictson
Copy link
Owner

I haven't had time to fully review this PR yet, but your comments about the side effects of decorate before debug? make me think that it is related to bug #43. Would you be willing to investigate?

@robd
Copy link
Contributor Author

robd commented Jul 7, 2015

Hey - yes I think you are right. I believe that #43 is definitely related and may well be fixed by moving the calls to decorate after the debug?(command). I can look into this later tonight. I think it should be possible to expose the behaviour via a test. I think what we are missing for #43 is a test case which runs info and debug in an interleaved way.

@robd
Copy link
Contributor Author

robd commented Jul 7, 2015

OK, I've had a look at this and you are right - #43 is due to calling decorate before debug. I have written a failing test for interleaved debug / info scenario. I think the refactor in this PR, or something very similar is a pre requisite to move the decorate after the debug call. Therefore, I will fix #43 on this branch. Hope that's OK.

@mattbrictson
Copy link
Owner

Sounds good! I'll wait for your patch and then merge this PR.

@robd
Copy link
Contributor Author

robd commented Jul 7, 2015

Closed in favour of #45

@robd robd closed this Jul 7, 2015
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.

None yet

2 participants