Conversation
🔍 Dependency Reviewgithub.com/bmatcuk/doublestar v1.3.4 -> v4.9.1 — ❌ Changes NeededSummary
Why
Changelog highlights (v1.3.4 → v4.9.1)
Code changes to apply
- matches, err := doublestar.Glob(targetPath)
+ matches, err := doublestar.FilepathGlob(targetPath)
-import "github.com/bmatcuk/doublestar"
+import (
+ "path/filepath"
+ "github.com/bmatcuk/doublestar/v4"
+)
- if match, _ := doublestar.PathMatch(exclude, m); match {
+ if match, _ := doublestar.PathMatch(filepath.FromSlash(exclude), m); match {
continue
}
- matches, err := doublestar.FilepathGlob(pattern)
+ matches, err := doublestar.FilepathGlob(pattern, doublestar.WithCaseInsensitive())
// internal/util/glob/glob.go
type Globber interface {
FilepathGlob(pattern string) ([]string, error)
PathMatch(pattern, path string) (bool, error)
}// internal/util/glob/glob_default.go (Unix – case-sensitive)
func NewGlobber() Globber { return &defaultGlobber{} }
func (g *defaultGlobber) FilepathGlob(p string) ([]string, error) { return doublestar.FilepathGlob(p) }
func (g *defaultGlobber) PathMatch(pat, path string) (bool, error) { return doublestar.PathMatch(pat, path) }// internal/util/glob/glob_windows.go (Windows – case-insensitive glob)
func NewGlobber() Globber { return &windowsGlobber{} }
func (g *windowsGlobber) FilepathGlob(p string) ([]string, error) {
return doublestar.FilepathGlob(p, doublestar.WithCaseInsensitive())
}
func (g *windowsGlobber) PathMatch(pat, path string) (bool, error) {
return doublestar.PathMatch(pat, path)
}
- matches, err := doublestar.Glob(w.getPath())
+ matches, err := w.globber.FilepathGlob(w.getPath())- if match, _ := doublestar.PathMatch(exclude, m); match {
+ if match, _ := w.globber.PathMatch(filepath.FromSlash(exclude), m); match {
continue
}Behavioral notes and tests to (re)confirm
References (click to view)
Notes
|
{...} expressions
|
💻 Deploy preview available (chore: Upgrade to doublestar/v4): |
d31b972 to
81be3f0
Compare
|
💻 Deploy preview deleted (feat(local.file_match, loki.source.file): Match multiple files using doublestar |
kalleep
left a comment
There was a problem hiding this comment.
Nice, great work. Just a few comments
| for _, m := range matches { | ||
| if exclude != "" { | ||
| if match, _ := doublestar.PathMatch(exclude, m); match { | ||
| if match, _ := s.globber.PathMatch(filepath.FromSlash(exclude), m); match { |
There was a problem hiding this comment.
Should we move filepath.FromSlash usage into globber(s)?
There was a problem hiding this comment.
I'm not sure. The reason I left it like this is so that it's more flexible if someone doesn't want the FromSlash in the future.
| } | ||
|
|
||
| func (g *windowsGlobber) FilepathGlob(pattern string) ([]string, error) { | ||
| return doublestar.FilepathGlob(pattern, doublestar.WithCaseInsensitive()) |
There was a problem hiding this comment.
Why an interface is required here? Is it necessary to do tests/mocks?
If the iface isn't strictly necessary, would suggest having a wrapper func that checks runtime.GOOS for case sensitivity option.
Downside of having separate build-constrained files is that syntax errors/linter/formatters may not run locally against build-constrained files for windows.
There was a problem hiding this comment.
It feels more defensive than using the doublestar functions directly.
If we use the functions directly and only switch to swapper funcs wherever is needed, then it would be very easy to mistakenly call the wrong function in case the code is refactored or extended.
|
Docs are fine as-is |
81be3f0 to
7a98372
Compare
7a98372 to
ffca308
Compare
kalleep
left a comment
There was a problem hiding this comment.
LGTM, one nit about the naming of glob_default but I don't feel too strongly about that one
Pull Request Details
Upgrading to doublestar/v4. The new version of the library is more performant and has the new
{...}matching syntax.However, it does have a few breaking changes. The breaking change which affects us the most is not documented - apparently the library is now case-sensitive by default even on Windows.
Issue(s) fixed by this Pull Request
Fixes #4423
Notes to the Reviewer
I checked the tests on Windows by removing the case sensitivity glob option and they indeed failed:
PR Checklist