I am concerned that many of these tests are now "passing", and don't
have any of the annotation for getting back to that were found. Given
how some of these failures are going to be fairly subtle, and that we
don't really do an exhaustive job of input testing, I worry that they
will end up missed...
That said, if those were already on your list of things to return to, great. :)
Daniel
On Tue, Jul 19, 2011 at 15:36, Josh Cooper <j...@puppetlabs.com> wrote:
> These set of changes are intended to get the tests working and the
> implementation in a stable state, though not strictly correct for the
> reasons you mention and also UNC paths, etc. Rather than make those changes
> in this patch series, I will be making a second pass, refactoring path
> validation into utility methods like we taked about.
> Josh
> On Tue, Jul 19, 2011 at 3:19 PM, Daniel Pittman <dan...@puppetlabs.com>
> wrote:
>>
>> On Tue, Jul 19, 2011 at 14:59, Josh Cooper <j...@puppetlabs.com> wrote:
>> > When testing whether a file path is absolute, the regexp was only
>> > handling POSIX style file paths. This commit requires Windows
>> > style file paths to start with a drive letter. A future commit
>> > will refacter the various places we do path validation to
>> > support both Windows drive letters and UNC paths.
>> >
>> > Reviewed-by: Jacob Helwig <ja...@puppetlabs.com>
>> > Signed-off-by: Josh Cooper <j...@puppetlabs.com>
>>
>> > - path = path.chomp(File::SEPARATOR) unless path == File::SEPARATOR
>> > - raise ArgumentError.new("Fileset paths must be fully qualified")
>> > unless File.expand_path(path) == path
>> > + if Puppet.features.microsoft_windows?
>> > + # REMIND: UNC path
>> > + path = path.chomp(File::SEPARATOR) unless path =~ /^[A-Za-z]:\/$/
>>
>> Doesn't this fail in the face of platforms that have both
>> File::SEPARATOR and File::ALT_SEPARATOR? It wouldn't strip the right
>> character from the end of the path, at least, in one of the two valid
>> cases.
>>
>> > + else
>> > + path = path.chomp(File::SEPARATOR) unless path == File::SEPARATOR
>> > + end
>>
>> Wouldn't it be better to have unified these tests, so that we don't
>> have to open-code these Windows-specific tests in every line of code
>> that needs to handle paths? That sounds like an invitation to a
>> constant stream of bugs where we fail to handle these cases to me.
>>
>> > def validate_dirs(dirs)
>> > + dir_regex = Puppet.features.microsoft_windows? ?
>> > /^[A-Za-z]:#{File::SEPARATOR}/ : /^#{File::SEPARATOR}/
>>
>> This will fail if File::SEPARATOR is '\', because you just shoved a
>> regexp escape character into your string. On the plus side, though,
>> because you are not actually getting it right WRT ALT_SEPARATOR, this
>> won't blow up. Two bugs make it work. ;)
>>
>> > - unless file =~ /^#{File::SEPARATOR}/
>> > + regex = Puppet.features.microsoft_windows? ?
>> > /^[A-Za-z]:#{File::SEPARATOR}/ : /^#{File::SEPARATOR}/
>>
>> Hey, I think I recognise this code. Maybe we should have somewhere
>> central for it, rather than open-coding it every place that needs it?
>>
>> Daniel
>> --
>> ⎋ Puppet Labs Developer – http://puppetlabs.com
>> ✉ Daniel Pittman <dan...@puppetlabs.com>
>> ✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775
>> ♲ Made with 100 percent post-consumer electrons
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Puppet Developers" group.
>> To post to this group, send email to puppet-dev@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.
>>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To post to this group, send email to puppet-dev@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.
>
--
⎋ Puppet Labs Developer – http://puppetlabs.com
✉ Daniel Pittman <dan...@puppetlabs.com>
✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775
♲ Made with 100 percent post-consumer electrons
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@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.
