Conversation
e939b4f to
395e228
Compare
395e228 to
699b325
Compare
|
Nice, thank you! This is looking good. Will merge this shortly. Appreciate your contribution! |
|
Hm, lz4 compression with extension is failing. @sorairolake Does it fail for you locally? |
|
@mholt Just now, I ran the test locally 10 times, 8 passed but 2 failed. I don't know why the test sometimes fails. |
|
Hmm, sounds like something funky with the test suite. I'll look into it, but your change seems sound. |
|
Both lz4 and lzip starts with "lz". So I think this may be related. |
| var mr MatchResult | ||
|
|
||
| // match filename | ||
| if strings.Contains(strings.ToLower(filename), lz.Name()) { |
There was a problem hiding this comment.
Ah, indeed -- make this comparison more strict and that should hopefully fix the test.
There was a problem hiding this comment.
I think this can be fixed with the following changes:
| if strings.Contains(strings.ToLower(filename), lz.Name()) { | |
| if filepath.Ext(strings.ToLower(filename)) == lz.Name() { |
However, this requires that the extension be the last element of the path.
There was a problem hiding this comment.
That would fail for those cases as you say, but maybe a check for both a suffix of .lz or the string contains .lz.?
There was a problem hiding this comment.
I think there is almost no possibility that .lz and .lz4 exists anywhere other than at the end of the path.
I think /path/to/foo.tar.lz or bar.7z.lz4 are possible, but /path/to/foo.lz.tar or bar.lz4.7z are almost impossible.
|
|
||
| // match filename | ||
| if strings.Contains(strings.ToLower(filename), lz.Name()) { | ||
| if filepath.Ext(strings.ToLower(filename)) == lz.Name() { |
There was a problem hiding this comment.
I thought this would be a conflict, but it was a misunderstanding. So I restored this.
4d2e93b to
82a374e
Compare
mholt
left a comment
There was a problem hiding this comment.
Awesome, looks good now :) Thank you!
|
PS. I hope to travel to Japan someday :D |
Closes #400