Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve ../ on pattern #21

Merged
merged 18 commits into from
Feb 28, 2021
Merged

fix: resolve ../ on pattern #21

merged 18 commits into from
Feb 28, 2021

Conversation

caarlos0
Copy link
Member

we had no tests for it, but it used to work on v0.

Also made the options accept a pattern as arg. Maybe we should even made MaybeRootFS default?

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0 caarlos0 self-assigned this Feb 27, 2021
@caarlos0 caarlos0 added automerge bug Something isn't working labels Feb 27, 2021
@codecov
Copy link

codecov bot commented Feb 27, 2021

Codecov Report

Merging #21 (2980661) into main (d5e84dd) will decrease coverage by 0.93%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
- Coverage   82.90%   81.96%   -0.94%     
==========================================
  Files           2        2              
  Lines         117      122       +5     
==========================================
+ Hits           97      100       +3     
- Misses         12       13       +1     
- Partials        8        9       +1     
Impacted Files Coverage Δ
glob.go 80.95% <80.00%> (-1.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e84dd...2980661. Read the comment docs.

@erikgeiser
Copy link
Member

erikgeiser commented Feb 28, 2021

Another approach would be to pass the pattern to compileOptions and save it on globOptions before the OptFunc are called. This way the OptFunc API stays clean and they can still access and even modify the pattern in the order they are specified.

I think the default should definitely be the a FS that starts at the actual OS filesystem root (or on windows the volume of the current dirrectory if the path is relative and the volume in the pattern if the path is absolute). I don't like the name MaybeRootFS and I find it weird that it may or may not replace a custom FS with a DirFS in the end. We could do a FS (maybe called RootFS) as discussed on Slack as the default value which is a DirFS rooted in / on posix systems, volume(cwd) for relative paths and volume(pattern) for absolute paths on windows. Also it should prepend the current working directory for relative paths on all systems before passing the path to the underlying DirFS.

If users use their custom FS, their globs will have to respect the quirks that their FS implementations come with.

Also what do we do about API stability as we already published a v1 tag? Should we redact it until the dust settles a bit on this or can we get away with breaking the API on a v1 version because it has just been a day since v1 was published?

@erikgeiser
Copy link
Member

I tried to implement my approach and now I see that the behavior of path argument of the WalkDirFunc makes all of it much more complicated. Unfortunately I don't have the time right now to fully work it out.

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 28, 2021
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0
Copy link
Member Author

Yeah breaking the api is a bad but in this case the v1 actually broke behavior we didn't intend to... imho we can bump feature and that should be ok.

your idea to put the pattern on the options was a good one, done ✅

the custom fs thing I also gave a try and seems like a very big thing to implement...

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0 caarlos0 merged commit 975308f into main Feb 28, 2021
@caarlos0 caarlos0 deleted the fix-parent branch February 28, 2021 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge bug Something isn't working size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants