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

Add wrapper for io/fs #81

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Add wrapper for io/fs #81

merged 5 commits into from
Sep 27, 2024

Conversation

evankanderson
Copy link
Contributor

Fixes #11

I did not implement SubFS or GlobFS, because those seemed a bit trickier, and I didn't need them for my use case. 😁 Another option would be to extend the billy.File interface to add a Stat() method, but that seemed like a breaking ("v6") change, at which point you might simply want to rewrite towards io/fs, which has a testing memory filesystem already.

It turns out that the Go fstest.TestFS is a bit more stringent than the billy tests,
and it checks to see whether two Stat() calls return the same results on the same file. I disabled those tests in the unit test, but we could also adjust the Memfs implementation to store the ModTimes on each file when they are created. I thought the extra in-memory storage probably wasn't worth it, but I have a patch if you'd prefer that approach.

ok  	github.com/go-git/go-billy/v5/helper/iofs	0.244s	coverage: 93.3% of statements

Note that you'll probably need Go 1.23.x to pick up this change to the errors returned by fstest.TestFS().

Signed-off-by: Evan Anderson <[email protected]>
@evankanderson
Copy link
Contributor Author

So, it looks like the FS test fails on 1.22 and earlier (as I mentioned in the original PR) because the ModTime on Stat() is always set to Now(). Would you like me to roll in a PR to fix that, or to add filtering for earlier versions of Go's errors?

@evankanderson
Copy link
Contributor Author

I added some line filtering that I hope will work for go <= 1.22. I haven't specifically set up a test environment, but I may try doing so if the workflows don't run in the next day or so.

@evankanderson
Copy link
Contributor Author

I got the chance to test on go 1.20.3 this evening, and discovered that the pre-1.23 error includes the line TestFS found errors:, so I handled that case as well.

@evankanderson
Copy link
Contributor Author

(I think this should pass CI now)

@pjbgf
Copy link
Member

pjbgf commented Sep 19, 2024

@evankanderson thanks for putting this PR together.

I got the chance to test on go 1.20.3 this evening, and discovered that the pre-1.23 error includes the line TestFS found errors:, so I handled that case as well.

go-git and go-billy are meant to only support the last 3 stable Go versions, so we don't need to worry about v1.20. However, we need a bump to get go.mod and tests to run on Go v1.21-v1.23. Feel free to add a commit to that effect.

helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Member

Choose a reason for hiding this comment

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

I would be good that tests take into account OS specific things, such as path separators:

Suggested change
"dir/baz.txt": "こんにちわ, world",
filepath.Join("dir","baz.txt"): "こんにちわ, world",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that fstest.testFS isn't written in a Windows-aware way (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/testing/fstest/testfs.go;l=147), so I skipped this test on Windows for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me sad, but not sad enough to re-implement fstest.testFS in this file, given that we have Linux coverage and billy coverage.

Copy link
Contributor Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback, I think I addressed all of it (and it should pass the Windows CI leg now).

helper/iofs/iofs.go Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
helper/iofs/iofs.go Outdated Show resolved Hide resolved
files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that fstest.testFS isn't written in a Windows-aware way (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/testing/fstest/testfs.go;l=147), so I skipped this test on Windows for now.

files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me sad, but not sad enough to re-implement fstest.testFS in this file, given that we have Linux coverage and billy coverage.

helper/iofs/iofs.go Outdated Show resolved Hide resolved
@evankanderson
Copy link
Contributor Author

I'm willing to bump go.mod and the tests, but I'd prefer to do it in a separate PR. (And this should pass on 1.20 if someone is using something that old).

helper/iofs/iofs.go Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

@evankanderson small change on the API to align with the rest of helpers, apart from that LGTM.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM. @evankanderson thanks for working on this. 🙇

@pjbgf pjbgf merged commit c1ee0b9 into go-git:master Sep 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: Relation to io/fs?
2 participants