On Tue, Jan 18, 2011 at 11:12 AM, Nick Lewis <[email protected]> wrote: > On Tue, Jan 18, 2011 at 10:44 AM, Nigel Kersten <[email protected]> > wrote: >> >> On Tue, Jan 18, 2011 at 10:41 AM, Nick Lewis <[email protected]> wrote: >> > I get an error as soon as I try to run this: >> > /Users/nicklewis/puppet/puppet/lib/puppet/defaults.rb:104: undefined >> > local >> > variable or method `diff' for Puppet:Module (NameError) >> > The Ruby interpreter is interpreting :show-diff as :show - diff. A >> > possible >> > fix would be to use :"show-diff", if this is what's really intended. >> > However, I don't have enough context from the ticket or commit message >> > to >> > understand the reason for this change. We have no options containing >> > dashes, >> > and many containing underscores, so I question what this change is >> > making >> > show_diff more consistent with. The only place we have dashes is in the >> > command-line form of specifying boolean options, in which case we use >> > --[no-]OPTION. In the actual option names, we are inconsistent between >> > underscores (show_diff, config_version) and no separator at all >> > (usecacheonfailure, downcasefacts). We do have an item on our technical >> > debt >> > list for standardizing to using hyphens in our option names (at least on >> > the >> > command line), but that will require a more general patch. >> >> --detailed-exit-codes doesn't count? >> >> I jumped the gun and assumed we'd already had to solve this due to >> other options containing hyphens. >> > > I should clarify that by "options" I only mean options that can be set in > puppet.conf, specified in defaults.rb. For every option in defaults.rb, we > add a corresponding command-line option to the option parser, using the > --[no-]OPTION syntax. Applications also add some of their own > application-specific command-line-only options (such as --debug, --verbose, > --detailed-exitcodes, --test), and do appear to use dashes. > I don't see any good reason we don't use dashes in defaults.rb, other than > the fact that we use symbols for our option names, which lend themselves > more easily to using underscores as seen in this patch. We could probably > start using strings and dashes fairly easily. Or we could convert > underscores to dashes in the process of creating the associated command-line > option, which requires fewer changes inside Puppet since the option names > themselves haven't changed, but also creates a disconnect between the option > name you use in puppet.conf and the option name you use on the command line. > Standardizing (and hyphenating) options is on our technical debt list, but > there are quite a few other items higher up the list. I'll file a ticket, so > we can properly prioritize it.
Given that, and that from looking at --genconfig we have quite a few underscores in config options and quite a few hyphens in application options, this shouldn't be merged. Thanks Nick. > Nick > >> >> > Nick >> > On Sun, Jan 16, 2011 at 4:28 PM, James Turnbull >> > <[email protected]> >> > wrote: >> >> >> >> Signed-off-by: James Turnbull <[email protected]> >> >> --- >> >> lib/puppet/application/agent.rb | 4 ++-- >> >> lib/puppet/application/apply.rb | 2 +- >> >> lib/puppet/defaults.rb | 2 +- >> >> lib/puppet/type/file/content.rb | 2 +- >> >> spec/unit/application/apply_spec.rb | 4 ++-- >> >> spec/unit/type/file/content_spec.rb | 4 ++-- >> >> 6 files changed, 9 insertions(+), 9 deletions(-) >> >> >> >> diff --git a/lib/puppet/application/agent.rb >> >> b/lib/puppet/application/agent.rb >> >> index 2b75505..291e68e 100644 >> >> --- a/lib/puppet/application/agent.rb >> >> +++ b/lib/puppet/application/agent.rb >> >> @@ -137,7 +137,7 @@ class Puppet::Application::Agent < >> >> Puppet::Application >> >> Puppet.settings.handlearg("--ignorecache") >> >> Puppet.settings.handlearg("--no-usecacheonfailure") >> >> Puppet.settings.handlearg("--no-splay") >> >> - Puppet.settings.handlearg("--show_diff") >> >> + Puppet.settings.handlearg("--show-diff") >> >> Puppet.settings.handlearg("--no-daemonize") >> >> options[:verbose] = true >> >> Puppet[:onetime] = true >> >> @@ -202,7 +202,7 @@ class Puppet::Application::Agent < >> >> Puppet::Application >> >> exit(Puppet.settings.print_configs ? 0 : 1) if >> >> Puppet.settings.print_configs? >> >> >> >> # If noop is set, then also enable diffs >> >> - Puppet[:show_diff] = true if Puppet[:noop] >> >> + Puppet[:show-diff] = true if Puppet[:noop] >> >> >> >> args[:Server] = Puppet[:server] >> >> if options[:fqdn] >> >> diff --git a/lib/puppet/application/apply.rb >> >> b/lib/puppet/application/apply.rb >> >> index 33a70ce..af1d08c 100644 >> >> --- a/lib/puppet/application/apply.rb >> >> +++ b/lib/puppet/application/apply.rb >> >> @@ -137,7 +137,7 @@ class Puppet::Application::Apply < >> >> Puppet::Application >> >> exit(Puppet.settings.print_configs ? 0 : 1) if >> >> Puppet.settings.print_configs? >> >> >> >> # If noop is set, then also enable diffs >> >> - Puppet[:show_diff] = true if Puppet[:noop] >> >> + Puppet[:show-diff] = true if Puppet[:noop] >> >> >> >> Puppet::Util::Log.newdestination(:console) unless options[:logset] >> >> client = nil >> >> diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb >> >> index 4521a59..ad82205 100644 >> >> --- a/lib/puppet/defaults.rb >> >> +++ b/lib/puppet/defaults.rb >> >> @@ -101,7 +101,7 @@ module Puppet >> >> }, >> >> :diff_args => ["-u", "Which arguments to pass to the diff command >> >> when >> >> printing differences between files."], >> >> :diff => ["diff", "Which diff command to use when printing >> >> differences >> >> between files."], >> >> - :show_diff => [false, "Whether to print a contextual diff when >> >> files >> >> are being replaced. The diff >> >> + :show-diff => [false, "Whether to print a contextual diff when >> >> files >> >> are being replaced. The diff >> >> is printed on stdout, so this option is meaningless unless you >> >> are >> >> running Puppet interactively. >> >> This feature currently requires the `diff/lcs` Ruby library."], >> >> :daemonize => { :default => true, >> >> diff --git a/lib/puppet/type/file/content.rb >> >> b/lib/puppet/type/file/content.rb >> >> index b8f30a9..2620bab 100755 >> >> --- a/lib/puppet/type/file/content.rb >> >> +++ b/lib/puppet/type/file/content.rb >> >> @@ -100,7 +100,7 @@ module Puppet >> >> >> >> result = super >> >> >> >> - if ! result and Puppet[:show_diff] >> >> + if ! result and Puppet[:show-diff] >> >> write_temporarily do |path| >> >> print diff(@resource[:path], path) >> >> end >> >> diff --git a/spec/unit/application/apply_spec.rb >> >> b/spec/unit/application/apply_spec.rb >> >> index 4e17442..c70e857 100755 >> >> --- a/spec/unit/application/apply_spec.rb >> >> +++ b/spec/unit/application/apply_spec.rb >> >> @@ -61,12 +61,12 @@ describe Puppet::Application::Apply do >> >> @apply.options.stubs(:[]).with(any_parameters) >> >> end >> >> >> >> - it "should set show_diff on --noop" do >> >> + it "should set show-diff on --noop" do >> >> Puppet.stubs(:[]=) >> >> Puppet.stubs(:[]).with(:config) >> >> Puppet.stubs(:[]).with(:noop).returns(true) >> >> >> >> - Puppet.expects(:[]=).with(:show_diff, true) >> >> + Puppet.expects(:[]=).with(:show-diff, true) >> >> >> >> @apply.setup >> >> end >> >> diff --git a/spec/unit/type/file/content_spec.rb >> >> b/spec/unit/type/file/content_spec.rb >> >> index cde643f..b48d57e 100755 >> >> --- a/spec/unit/type/file/content_spec.rb >> >> +++ b/spec/unit/type/file/content_spec.rb >> >> @@ -169,9 +169,9 @@ describe content do >> >> @content.must be_insync("{md5}" + Digest::MD5.hexdigest("some >> >> content")) >> >> end >> >> >> >> - describe "and Puppet[:show_diff] is set" do >> >> + describe "and Puppet[:show-diff] is set" do >> >> before do >> >> - Puppet[:show_diff] = true >> >> + Puppet[:show-diff] = true >> >> end >> >> >> >> it "should display a diff if the current contents are different >> >> from the desired content" do >> >> -- >> >> 1.7.3.4 >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "Puppet Developers" group. >> >> To post to this group, send email to [email protected]. >> >> To unsubscribe from this group, send email to >> >> [email protected]. >> >> For more options, visit this group at >> >> http://groups.google.com/group/puppet-dev?hl=en. >> >> >> > >> > -- >> > You received this message because you are subscribed to the Google >> > Groups >> > "Puppet Developers" group. >> > To post to this group, send email to [email protected]. >> > To unsubscribe from this group, send email to >> > [email protected]. >> > For more options, visit this group at >> > http://groups.google.com/group/puppet-dev?hl=en. >> > >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Puppet Developers" group. >> To post to this group, send email to [email protected]. >> To unsubscribe from this group, send email to >> [email protected]. >> For more options, visit this group at >> http://groups.google.com/group/puppet-dev?hl=en. >> > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
