Skip to content
This repository has been archived by the owner on Mar 19, 2022. It is now read-only.

UI messaging is inconsistant during cook command #215

Merged
merged 2 commits into from Mar 1, 2013

Conversation

dwradcliffe
Copy link
Contributor

There is one ui message for checking the chef version, and one debug message, and the rest of the parts don't give any messaging. This should be standardized. IMO, a ui message for each step would be nice to keep me updated. I'm working on a PR for this now.

@matschaffer
Copy link
Owner

Agreed! There are those timing blocks around a bunch of stuff. Maybe it'd
be worth generalizing them into "steps" that had both logging and timing
information associated with them.

-Mat

about.me/matschaffer

On Thu, Feb 28, 2013 at 9:03 AM, David Radcliffe
notifications@github.comwrote:

There is one ui message for checking the chef version, and one debug
message, and the rest of the parts don't give any messaging. This should be
standardized. IMO, a ui message for each step would be nice to keep me
updated. I'm working on a PR for this now.


Reply to this email directly or view it on GitHubhttps://github.com//issues/215
.

@tmatilai
Copy link
Collaborator

+1. Or +2 if the same standard is applied to other commands (at least prepare), too.

@dwradcliffe
Copy link
Contributor Author

Something like this?

step "Validating options..." do
  validate!
end
step "Bootstrapping Chef..." do
  bootstrap.bootstrap!
end
step "Generating node config..." do
  generate_node_config
end

Or should I move the call to step to each method?

@matschaffer
Copy link
Owner

That feels a little verbose, but could work. What about extracting the
strings?

step :validate { validate! }
step :bootstrap { bootstrap.bootstrap! }

Could make i18n easier down the road.

-Mat

about.me/matschaffer

On Thu, Feb 28, 2013 at 9:32 AM, David Radcliffe
notifications@github.comwrote:

Something like this?

step "Validating options..." do
validate!endstep "Bootstrapping Chef..." do
bootstrap.bootstrap!endstep "Generating node config..." do
generate_node_configend

Or should I move the call to step to each method?


Reply to this email directly or view it on GitHubhttps://github.com//issues/215#issuecomment-14235722
.

@matschaffer
Copy link
Owner

Makes me wish this were clojure. (step validate) would totally be enough.

-Mat

about.me/matschaffer

On Thu, Feb 28, 2013 at 9:34 AM, Mat Schaffer mat@schaffer.me wrote:

That feels a little verbose, but could work. What about extracting the
strings?

step :validate { validate! }
step :bootstrap { bootstrap.bootstrap! }

Could make i18n easier down the road.

-Mat

about.me/matschaffer

On Thu, Feb 28, 2013 at 9:32 AM, David Radcliffe <notifications@github.com

wrote:

Something like this?

step "Validating options..." do
validate!endstep "Bootstrapping Chef..." do
bootstrap.bootstrap!endstep "Generating node config..." do
generate_node_configend

Or should I move the call to step to each method?


Reply to this email directly or view it on GitHubhttps://github.com//issues/215#issuecomment-14235722
.

@dwradcliffe
Copy link
Contributor Author

@matschaffer Using the i18n gem or something hand rolled?

@matschaffer
Copy link
Owner

The gem would be worth a look. But if you think it'll get in the way of
finishing the PR, a hash would be a fine start.

Also. How does knife-ec2 do their logging? Maybe we should just mimic that?

On Feb 28, 2013, at 9:47, David Radcliffe notifications@github.com wrote:

@matschaffer https://github.com/matschaffer Using the i18n gem or
something hand rolled?


Reply to this email directly or view it on
GitHubhttps://github.com//issues/215#issuecomment-14236414
.

@tmatilai
Copy link
Collaborator

knife-ec2 uses simply puts with color codes. I think I'm declined to stay with the ui object. Then the user should have more control on log level.

@matschaffer
Copy link
Owner

That's a sound call. You think there's value in a "step" abstraction? As
much as I love to abstract, it could be YAGNI.

-Mat

about.me/matschaffer

On Thu, Feb 28, 2013 at 10:15 AM, Teemu Matilainen <notifications@github.com

wrote:

knife-ec2 uses simply puts with color codes. I think I'm declined to stay
with the ui object. Then the user should have more control on log level.


Reply to this email directly or view it on GitHubhttps://github.com//issues/215#issuecomment-14237919
.

@dwradcliffe
Copy link
Contributor Author

After starting to implement, I'm thinking it might be unnecessary. There are enough cases where logic is in the methods (like generate_node_config) that make this a bit awkward.

That said, I can go either direction.

@matschaffer
Copy link
Owner

@dwradcliffe trust your instincts. Looking forward to seeing what you come up with! :)

@tmatilai
Copy link
Collaborator

I didn't get to test it yet but looks good! Anyway I think that the validate! method does not need a message. It will bark in case of problems and it's lighning fast anyways.

@dwradcliffe
Copy link
Contributor Author

@tmatilai Good call, I've removed it.

@matschaffer
Copy link
Owner

Looks good, but I get this test error for some reason: https://gist.github.com/matschaffer/5064531

@dwradcliffe
Copy link
Contributor Author

@matschaffer I used #{host} in one of the messages before validating the options. Just pushed a fix. Tests pass for me now.

@matschaffer
Copy link
Owner

Sweet, thanks!

matschaffer added a commit that referenced this pull request Mar 1, 2013
UI messaging is inconsistant during cook command
@matschaffer matschaffer merged commit d6ab51f into matschaffer:master Mar 1, 2013
@dwradcliffe dwradcliffe deleted the command-messaging branch March 1, 2013 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants