gowebdav icon indicating copy to clipboard operation
gowebdav copied to clipboard

Returns pointer to FileInfo from ReadDir

Open the-plate opened this issue 2 years ago • 11 comments

Returns pointer to os.FileInfo from ReadDir() - same as Stat does. This would than ensure compatibility of return types for Stat() and ReadDir(). Now are different.

the-plate avatar Oct 18 '23 14:10 the-plate

Since the methods of the interface implementation do not have a pointer receiver shouldn't Stat() return the struct itself instead of a pointer to the struct?

ueffel avatar Oct 18 '23 17:10 ueffel

What implementation do you mean exactly? I can see few having pointer receiver.

the-plate avatar Oct 18 '23 20:10 the-plate

https://github.com/studio-b12/gowebdav/blob/master/file.go I mean the implementation of os.FileInfo by gowebdav.File

ueffel avatar Oct 18 '23 20:10 ueffel

Ok I see. On one hand, I'm bit confused, the File struct in file.go does not implement the mentioned Stat() function. And on the other hand, it it possible to call value receiver function on a pointer value. The pointer in such case is automatically dereferenced. On top of that the File implements only, say, getters so the pointer receiver is not necessary and value receiver can be used.

the-plate avatar Oct 19 '23 08:10 the-plate

Hello, any comments on this topic ? Thanks!

the-plate avatar Nov 02 '23 12:11 the-plate

Hi @ueffel could you please comment on this? Thanks

the-plate avatar Mar 03 '24 22:03 the-plate

Ah, I got my trail of thought again:

Returning *gowebdav.File (pointer to struct) as os.FileInfo (interface) has an unnecessary level of indirection. Returning a struct (gowebdav.File) as interface (os.FileInfo) already makes a pointer out of it.

To align the return types of ReadDir and Stat, Stat should be changed, not ReadDir:

 // Stat returns the file stats for a specified path
 func (c *Client) Stat(path string) (os.FileInfo, error) {
-       var f *File
+       var f File
        parse := func(resp interface{}) error {
                r := resp.(*response)
-               if p := getProps(r, "200"); p != nil && f == nil {
-                       f = new(File)
+               if p := getProps(r, "200"); p != nil {
+                       f = File{}
                        f.name = p.Name
                        f.path = path
                        f.etag = p.ETag

@chripo thoughts?

ueffel avatar Mar 05 '24 22:03 ueffel

I'm think @ueffel is right with the unnecessary level of indirection. Maybe we should refactor ReadDir to f = File{} too?

chripo avatar Mar 13 '24 17:03 chripo

Yeah, makes sense.

ueffel avatar Mar 13 '24 18:03 ueffel

Thanks for comments, will you make the changes?

the-plate avatar Mar 13 '24 20:03 the-plate

@the-plate feel free to make changes!

chripo avatar Mar 13 '24 20:03 chripo