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

feat: replacing afero.Fs with fs.FS #17

Merged
merged 59 commits into from
Feb 27, 2021
Merged

feat: replacing afero.Fs with fs.FS #17

merged 59 commits into from
Feb 27, 2021

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Feb 3, 2021

BREAKING CHANGE: stop using afero.Fs and use the new fs.FS package instead.

Also improves windows support all over the place.

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0
Copy link
Member Author

caarlos0 commented Feb 3, 2021

Not to be merged before go 1.16 stable is out...

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0
Copy link
Member Author

caarlos0 commented Feb 3, 2021

setup-go action don't support go tip 😞

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>
@pull-request-size pull-request-size bot added size/M and removed size/L labels Feb 4, 2021
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
glob.go Outdated
@@ -80,14 +83,21 @@ func toNixPath(path string) string {

// Glob returns all files that match the given pattern in the current directory.
func Glob(pattern string, opts ...OptFunc) ([]string, error) {
prefix := "./"
if strings.HasPrefix(pattern, stringSeparator) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure about this part yet... DirFS won't allow to start on /, so to keep the API I'm doing this little hack here... but maybe we can just declare it a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by that? prefix = stringSeparator and WithFS(os.DirFS(prefix)) seems like you are doing exactly that.

As far as I understand the docs, os.DirFS has no concept of a current working directy. So normally I would expect the fs to be os.DirFS("/") so I can access everything. But if I open file I would expect it to open /cwd/file where /cwd is my current working directory. But instead, os.DirFS("/") opens /file`. So what we would need to keep track of is the current working directory right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs.Open only accepts relative dirs

but I think this is indeed outside of the API.

Before it was also only working on paths relative to current, and didn't accept absolute paths anyway (/etc/foo/bar.conf for example)

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 7, 2021
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>
@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #17 (225313c) into main (71ebeed) will decrease coverage by 0.76%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   83.67%   82.90%   -0.77%     
==========================================
  Files           2        2              
  Lines          98      117      +19     
==========================================
+ Hits           82       97      +15     
- Misses          9       12       +3     
- Partials        7        8       +1     
Impacted Files Coverage Δ
glob.go 82.27% <88.57%> (-1.06%) ⬇️

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 71ebeed...225313c. Read the comment docs.

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
Copy link
Member Author

@erikgeiser I think its good to be reviewed again.

I make the Root path stuff an Option (and also added a "print options" option, for debugging et al).

Basically, by default, we start in current working dir (.) (as before). If the API user wants, they can use the MaybeRootFS option to detect that and work accordingly.

Prefix is needed in the case of "RootFS" because all results will be relative to the FS (in this case, /), so the results will be etc/foo.conf instead of /etc/foo.conf, for example, which may cause confusion.

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>
This reverts commit 2862f40.
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
This reverts commit ffb6804.
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
@caarlos0
Copy link
Member Author

caarlos0 commented Feb 27, 2021

at the cost of my sanity, it is now working on windows

Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants