[VFS] Update file content #48
Conversation
jinroh
force-pushed
the
vfs-update-file-content
branch
from
2c6147b
to
749556f
Compare
jinroh
force-pushed
the
vfs-update-file-content
branch
from
749556f
to
db5215f
Compare
| if err != nil { | ||
| // NewDirDoc is the DirDoc constructor. The given name is validated. | ||
| func NewDirDoc(name, folderID string, tags []string) (doc *DirDoc, err error) { | ||
| if err = checkFileName(name); err != nil { |
There was a problem hiding this comment.
Coding style decision to be made here :
@nono reverted this kind of set-output-value-and-return-empty in one of its PR (although it was only 1 value)
We need to pick a style and stick to it. Personnaly, I am not sure : this is cleaner than return nil, err but the intent is less readable. When in doubt, it is better to pick more readability rather than a few less chars.
There was a problem hiding this comment.
Sure.. I find named returns useful when I change the signature of a function (to have smaller diffs too..). But in the end I don't really care that much. @nono an opinion ?
There was a problem hiding this comment.
Here I would have to do return "", nil, err which I think is a bit annoying..
Although for functions returning only 0 or 1 value + error, return nil, err may be a bit cleaner...
There was a problem hiding this comment.
I like to give a name to return values to explain what they are. But I don't like the naked returns. It's really too easy to shadow err by mistake. This is an example of what I means: https://play.golang.org/p/0ZZ7bXElXq.
| func copyOnFsAndCheckIntegrity(m *DocAttributes, fs afero.Fs, pth string, r io.ReadCloser) (written int64, md5Sum []byte, err error) { | ||
| f, err := fs.Create(pth) | ||
| func copyOnFsAndCheckIntegrity(pth string, givenMD5 []byte, executable bool, fs afero.Fs, r io.Reader) (written int64, md5Sum []byte, err error) { | ||
| // We want to write only (O_WRONLY), create the file if it does not |
There was a problem hiding this comment.
This comment is at the wrong place (should be before L279). Another comment about "magic numbers" 0755 & 644 would be cool.
| return | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
The defer call should be before fs.Chmod, no?
There was a problem hiding this comment.
You're right, done 108945b
| func getFsAndDBPrefix(c *gin.Context) (fs afero.Fs, dbPrefix string, err error) { | ||
| instance := middlewares.GetInstance(c) | ||
| dbPrefix = instance.GetDatabasePrefix() | ||
| fs, err = instance.GetStorageProvider() |
There was a problem hiding this comment.
[Future] May be all this instanciating of request specific structs (instance, dbprefix, fs) could be moved to middlewares and c.MustGet.
| @@ -332,7 +443,9 @@ func TestMain(m *testing.M) { | |||
| router.Use(injectInstance(instance)) | |||
| router.POST("/files/", CreationHandler) | |||
| router.POST("/files/:folder-id", CreationHandler) | |||
| router.GET("/files/:file-id", ReadHandler) | |||
| router.PUT("/files/:file-id", OverwriteFileContentHandler) | |||
| router.HEAD("/files/:file-id", ReadFileHandler) | |||
There was a problem hiding this comment.
I'm curious. Do you know what we have to do to support HEAD with Gin?
There was a problem hiding this comment.
I have not seen anything specific in gin to "support" HEAD. This HEAD route is totally backed up by http.ServeContent: https://github.com/golang/go/blob/ad26bb5e3098cbfd7c0ad9a1dc9d38c92e50f06e/src/net/http/fs.go#L266-L270
This PR introduce the new route
PUT /files/:files-idalong with a new methodvfs.ModifyFileContent.Some notes:
In order to add this feature, I also removed the intermediary (and awkward) struct
vfs.DirAttributesthat was used to stack all the contents of a creation request for files and directories. Now,web/filecan usevfs.NewFileDocorvfs.NewDirDocdirectly and appropriately.I also increased the gometalinter dupl-threshold, because it annoyed me on some trivial code (in test)...