717 submit enormous files#725
Conversation
nywilken
left a comment
nywilken
left a comment
There was a problem hiding this comment.
@sfairchild thanks again for your recent contributions 🙏🏽
I left a few suggestions and called out a couple of gotchas to look out for. Please let me know if you have questions.
| if err != nil { | ||
| return err | ||
| } | ||
| if info.Size() >= int64(65535) { |
There was a problem hiding this comment.
You can turn this into an unxported constant const maxFileSize int64 = 65535
| if info.Size() >= int64(65535) { | ||
| msg :=` | ||
|
|
||
| The file you are trying to submit is %d bytes. Please reduce the file to below 65535 bytes and try again. |
There was a problem hiding this comment.
Small suggestion on the messaging
The submitted file is larger than the max allowed file size of %d bytes. Please reduce the size of the file and try again.
We can use the const here as well for printing and test explicitly for this message.
This error will also throw golint error because the error message starts with a capital letter, but that’s okay right now because of how we are printing this errors. We’ll solve that later for submit and download.
| err = ioutil.WriteFile(file, make([]byte, 65535), os.FileMode(0755)) | ||
|
|
||
| err = runSubmit(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), []string{file}) | ||
|
|
There was a problem hiding this comment.
Let’s assert that we have an error assert.EqualError. I’ve seen cases in the past where the error check was skipped, and then resulted in a nil reference error when calling err.Error(). Which cluters the test output making it just a little longer to debug.
|
|
||
| err = runSubmit(cfg, pflag.NewFlagSet("fake", pflag.PanicOnError), []string{file}) | ||
|
|
||
| assert.Regexp(t, "Please reduce the file to below 65535 bytes and try again.", err.Error()) |
There was a problem hiding this comment.
See my suggestion above about the messaging and using a const variable.
|
Thank you @nywilken. Go isn't my primary language and I just started learning it. I updated based on your comments and let me know if you have other concerns. |
nywilken
left a comment
nywilken
left a comment
There was a problem hiding this comment.
@sfairchild glad to help get this merged in. Thanks for the contribution. I left one more comment pertaining to the name of the constant, but this is solid. I tested locally will files of various sizes and all works as expected.
| if err != nil { | ||
| return err | ||
| } | ||
| const MaxFileSize int64 = 65535 |
There was a problem hiding this comment.
In Go variable names starting with a capital letter usually denote exported variables (shared across the package). Given the scope in which this constant is defined it can not be exported.
In an attempt to keep things inline with the Code review section for Go https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps this const should be called maxFileSize.
2 participants
Closes #717
Throws an error if file is larger than 65535 bytes