-
Notifications
You must be signed in to change notification settings - Fork 24
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
adding ParseMax w/ max form size in signature - forwarded to from Parse #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left one small comment. After it's addressed and I've confirmed the tests pass, I'll merge.
@@ -15,6 +15,8 @@ import ( | |||
"strings" | |||
) | |||
|
|||
const DefaultMaxFormSize = 2048 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like for all exported identifiers to have doc comments. Mind adding a comment here? It doesn't have to be long. Just something like:
// DefaultMaxFormSize is the default maximum form size (in bytes) used by the Parse function.
Also, I know this is my fault, but I think 2048 is probably much too small. I want to find a value that is likely to be safe (i.e. is unlikely to cause OOM exceptions in most environments) and allows for common things like text files, pdfs, and smaller images. Shall we try something around 500 KB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. I'll update in a bit! Thanks
@albrow just made your changes. I also added a |
@dadleyy thanks for updating this PR. I'm sorry for the delay, but my main development computer has been out of commission for a while. I'll likely be up and running again in the next week or so and I can do a more thorough review at that time.
Thank you for this :) I have nothing against Travis but I've been using CircleCI for many of my other projects (albrow/zoom for example) and I'm more familiar with it. I'll leave .travis.yaml there for now but I'll likely remove it after I set up CircleCI. |
Thanks again, @dadleyy! I merged this and released version 0.4.0. |
closes #3