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

WIP: fs.FS support #175

Closed
wants to merge 1 commit into from
Closed

Conversation

aol-nnov
Copy link
Contributor

@aol-nnov aol-nnov commented Apr 3, 2023

@deitch please find my first effors of implementing fs.FS here. Only iso9660 is supported as yet.

For me, there it is a dilemma: either to break the api and implement whole fs.FS support or implement wrapper functions. Wrappers look clumsily and, either way, require modifications of existing functions...

As for newly created diskfs.OpenFile, I've made it during my efforts of fiddling with "multi-layered fs.FS", i.e. say, I have a DirFS, where I call fs.Open yeiding an fs.File, which I pass to diskfs.OpenFile to get to the next layer. Again, it's not very handy, as diskfs.initDisk requires os.File - another "why" in my journey..

closes #169

@aol-nnov aol-nnov changed the title WIP: fs.FS support for iso9660 WIP: fs.FS support Apr 3, 2023
@deitch
Copy link
Collaborator

deitch commented Apr 4, 2023

So each filesystem would implement fs.FS?

For me, there it is a dilemma: either to break the api and implement whole fs.FS support or implement wrapper functions. Wrappers look clumsily and, either way, require modifications of existing functions

I am not sure what you mean by this. Can you show a practical example?

@aol-nnov
Copy link
Contributor Author

aol-nnov commented Apr 4, 2023

So each filesystem would implement fs.FS?

Yes, that's right! And please find the practical example in the related issue comment.

@deitch
Copy link
Collaborator

deitch commented Apr 4, 2023

I still don't get the wrapper issue?

@aol-nnov
Copy link
Contributor Author

aol-nnov commented Apr 4, 2023

Okay I'll elaborate more when I get to my laptop. I'm on the road now...

@aol-nnov
Copy link
Contributor Author

aol-nnov commented Apr 4, 2023

So, my thoughts on the implementation, as I see it. I may be wrong in some points as I'm still new to go.

As you can see from my wip patchset, there are very few receiver methods missing for the diskfs.Filesystem struct to pass the .(fs.FS) type assertion. The same stands for other fs-related structs.

  1. diskfs.File was missing only ReadDir() and Stat() to look like fs.File
  2. diskfs.Filesystem was missing Open() to look like fs.FS
  3. and iso9660.directoryEntry was missing Type() and Info() to look like os.FileInfo, plus it has its receivers implemented as pointer receivers instead of value receivers. It did not work out at some point and I had to change them to value receivers.

Having such minor changes here I'm not tempted to write a wrapper, to be honest.

Now to incompatibilities:
diskfs.Filesystem OpenFile receiver works almost as fs.FS Open should, but it should operate on directories, too. Plus, it must open an root directory of a filesystem when the path is . (as other fs implemetations do) and having two separate methons: for Open and OpenFile would be a 90% copy-paste of the latter.

Also, it is tempting to have ReadDirFS implemented, but diskfs.Filesystem ReadDir does not have proper return types for that..

So, here are my thoughts, pros and cons... Again, I might be totally wrong or missing something obvious, so please let me know!

P.S.: I suspect, that having ReadDirFs implemented in diskfs.Filesystem will eliminate the need of ReadDir() in diskfs.File, but this statement needs a check ;)

@deitch
Copy link
Collaborator

deitch commented Apr 9, 2023

Now I understand where you are going.

I think you are in the right direction. My only issue is something that is not the fault of your implementation, but actually of go in general.

I cannot implement an interface by returning something that implements an interface, only the interface itself. Specifically, if fs.FS requires Open(name string) (fs.File, error), then I must return actual fs.File, and not "something that implements fs.File". That means I cannot return my own thing. I cannot even return some other interface that includes this interface.

As an example, see here. Despite the fact that impl has Hello() B, which returns B, and B includes A, it will will not accept it.

So if I want to return diskfs.File, that will not work. I must return fs.File.

I am not sure I like that restriction. That may push us towards the idea of a wrapper. So you could have:

filesystem.Open() filesystem.File

And maybe a wrapper, so you get (pseudo-code):

fsys := diskfs.Open()
fsys.FS()   // convert to fs.FS

The reason I am concerned about having Open() just return an fs.File, is that it is quite limited:

type File interface {
	Stat() (FileInfo, error)
	Read([]byte), (int, error)
	Close() error
}

Whereas we have ReadWriteSeeker and ReaderAt. And there is no way to return filesystem.File, even if it fully implements fs.File.

@AndreRenaud
Copy link
Contributor

Given that generics have landed now, it might be possible to use the existing types internally, and then use a generic to convert them to fs.FS compatible types at compile time?

ie: https://play.golang.com/p/AHrNgLNYggU

This is a bit convoluted, but it does get around the issue that interfaces which define functions that return interfaces are not satisfied if an implementation returns a concrete type (that implements the correct interface).

@AndreRenaud
Copy link
Contributor

I had a play with the generics idea, and it didn't play out as nicely as I'd hoped. But using a wrapper type seems to work ok - would this approach be simpler? #184

@deitch
Copy link
Collaborator

deitch commented Jun 4, 2023

Yeah, I like what you did in #184 , let's move the detailed conversation over to there.

@aol-nnov
Copy link
Contributor Author

@AndreRenaud glad you came up with less invasive solution, thank you!

@deitch do you plan cutting a release any time soon? It's been almost a year since 1.3.0 and I'd be more than happy to see this feature included into the nearest release!

@aol-nnov aol-nnov closed this Aug 20, 2023
@deitch
Copy link
Collaborator

deitch commented Aug 21, 2023

Yeah, we can, I see no reason not to.

Done

@aol-nnov
Copy link
Contributor Author

Thank you for the prompt response, @deitch !

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.

Implement standard fs.FS interface and friends
3 participants