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

Fix inter-woven output during start #225

Merged
merged 2 commits into from
Jul 25, 2014
Merged

Fix inter-woven output during start #225

merged 2 commits into from
Jul 25, 2014

Conversation

tedsuo
Copy link

@tedsuo tedsuo commented Jul 25, 2014

We now wait until log messages are fully flushed before displaying instance status.
Also, we switched the FakeLogsRepository to be generated by counterfeiter.

Ted Young added 2 commits July 25, 2014 11:23
Signed-off-by: Aram Price <aprice@pivotallabs.com>
Signed-off-by: Aram Price <aprice@pivotallabs.com>
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/75733456.

crhino added a commit that referenced this pull request Jul 25, 2014
Fix inter-woven output during start
@crhino crhino merged commit 4e62760 into master Jul 25, 2014
@crhino
Copy link
Contributor

crhino commented Jul 25, 2014

Looks good. One note: we're preferring to say things like

startChan chan bool, doneChan chan bool

rather than

startChan, doneChan chan bool

partially for legibility and partially because the second form confuses counterfeiter.

Not a big enough deal to block the merge, though.

@tedsuo
Copy link
Author

tedsuo commented Jul 26, 2014

@crhino, counterfeiter is no longer confused by that pattern FYI. Thanks!

@tedsuo tedsuo deleted the flush-log-messages branch July 26, 2014 00:51
@lvarvel
Copy link
Contributor

lvarvel commented Jul 26, 2014

@tedsuo - Well then, I'll just have to dislike it for purely aesthetic reasons. Your PR is still merged. :P

haydonryan pushed a commit to haydonryan/cli that referenced this pull request Nov 7, 2014
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.

4 participants