Include a blank line after the first (subject) line of your commit to
keep the whole commit message from widing up in the subject line.
- Added WINDOWS constant that can be used as a boolean flag for other code.
- Added INTERPRETER constant that is set to '/bin/sh' on Unix, and
cmd.exe on MS Windows.
I'd rather call these MSWindows and Shell respectively, and the first
at least should probably live in Config instead of in
Facter::Util::Resolution.
- Removed the have_which method in favor of a pure Ruby which method
that can be used on any platform.
- The second argument defaults to the value of the INTERPRETER
constant, as does the error check.
- The system call to which has been replaced with the pure Ruby implementation.
- The way in which the binary name is determined has been modified to
handle the possibility of spaces in the path name.
> + if ENV['PATHEXT']
> + extensions = ENV['PATHEXT'].split(';').map{ |e| e.downcase }
> else
> + extensions = %w[.exe .com .bat]
> end
This should be passed in via Config::CONFIG instead of the
environment and defaulted more like:
extensions = (Config::CONFIG['path_ext'] ||
".exe;.com;.bat").downcase.split(';')
(the if isn't needed if you default before splitting, nor is the map
if you downcase after defaulting but before splitting).
> + if File.extname(program).empty?
> + unless extensions.include?(File.extname(program).downcase)
> + extensions.each{ |ext|
> + programs.push(program + ext)
> + }
> + end
> end
I'm confused by this. We only check if the extension is in the
extensions if there isn't one?
> + end
> +
> + # Catch the first path found, or nil
> + location = catch(:done){
> + path.split(File::PATH_SEPARATOR).each{ |dir|
> + programs.each{ |prog|
> + f = File.join(dir, prog)
> + if File.executable?(f) && !File.directory?(f)
> + location = File.join(dir, prog)
> + location.tr!('/', File::ALT_SEPARATOR) if
> File::ALT_SEPARATOR
> + throw(:done, location)
> + end
> + }
> + }
Catch and throw? Seriously? That seems like overkill.
Also, going to a pure ruby which is potentially a behavior change for
non-MS Windows platforms; this is the sort of thing that leads to
subtle bugs. I'd rather we either modify the command to explicitly
include the path we found or continue using the system which on *nix.
> + binary = File.join(File.dirname(code),
> File.basename(code).split.first)
I'm not clear on the reason behind this change; specifically (assuming
it's the "handle spaces" change) how it's supposed to work with
explicit paths in the "Program Files" directory.
> + path = nil
> +
> + # Don't look for the binary if an absolute path is provided
> + if binary !~ /^\/|^\w:[\/\\]/i
> + path = self.which(binary)
> + return nil if path.nil?
> + else
> + path = binary
> end
>
> + return nil unless FileTest.exists?(path)
> +
In other words,
binary = which(binary) unless binary =~ /^\/|^\w:[\/\\]/i
return unless binary && FileTest.exists?(binary)
> out = nil
> +
> ...etc
Please don't add random blank lines. Our code is frothy enough
already (conversely, feel free to remove them where they aren't adding
value).
-- Markus
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
