Implement globby.GlobFiles using doublestar.GlobWalk #1108
Conversation
|
@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
5ec8590 to
674f20f
Compare
45ab886 to
43a7bcb
Compare
43a7bcb to
0a7c828
Compare
This change aims to improve the performance of our file walking by only walking the portions of the directory that we care about. In this iteration it aims to be 100% backwards compatible with the previous implementation, excepting one obvious bug, resulting in a boolean change to the tests. It also adopts a set instead of a range for storage as doublestar will happily walk a directory multiple times if you feed it a nonsensical glob pattern such as `**/**/**/*.*`.
1c60a02 to
a199d1d
Compare
a199d1d to
8454b15
Compare
999a3c6 to
34dbeb0
Compare
| } | ||
|
|
||
| // isRelativePath ensures that the the requested file path is a child of `from`. | ||
| func isRelativePath(from string, to string) (isRelative bool, err error) { |
There was a problem hiding this comment.
This is somewhat a judgment call and I didn't find anything explicitly in the style guide, so push back if you disagree (also not really a blocker):
The community has a range of opinions on named return values, and their sometimes-companion, the naked return[0][1]. I think it's unfortunate that the documentation aspects incur the downsides that enable naked return. As a result, my preference is to avoid named return values altogether, as I think they tend to end up less readable and subject to unintentional shadowing.
I did find a linter, nakedret that catches half of this problem, and I would be happy to enable if you're on board.
In this particular case, I can see the intention to label a bool return value. My opinion is that the function name and associated comment is sufficient, but therein lies the judgment call. I also think you have shadowed err via := assignment, which works out in this case, but may not have been your intention(?).
Sample of opinions:
0 : https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_named_return_values
1 : golang/go#21291
There was a problem hiding this comment.
I ... did not fully understand how that worked when I wrote it. 😅 I was treating it as a "typed tuple" for the return values with "naming hints" without needing to go so far as actually creating a struct.
I believe naked returns in a function with any branching is not a particularly great idea and prefer explicit over implicit. I'd love to enable nakedret with l set to something silly like 3. ("If you can fit a naked return in a function this size, you know what, have at it.")
I also found shadow which might help me not shoot my foot off:
go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow@latest
go vet -vettool=$(which shadow) globby.go # 0 right now
Thoughts on somehow enabling that as a linter?
I'm going to move this away from named return values, I agree that it's really easy to accidentally stumble into a minefield with this approach.
This PR is a fast-follow to #1108 to plumb through the error cases. This work is intentionally split to reduce the scope for each change (1. change implementation, 2. change interface). This results in user-facing changes if they have globs or patterns that push them outside of their root directory.
This change aims to improve the performance of our file walking by only walking the portions of the directory that we care about. In this iteration it aims to be 100% backwards compatible with the previous implementation, excepting one obvious bug, resulting in a boolean change to the tests.
It also adopts a set instead of a range for storage as doublestar will happily walk a directory multiple times if you feed it a nonsensical glob pattern such as
**/**/**.