-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Content-Type header parser #259
Conversation
I don't know why the tests fail. It also fails on devel even without my changes. And the process seems to hang. |
The devel actually passes the tests because circle CI is unused though it still causes failures. I guess there is a loop which causes the hang |
Out of curiosity, what blocks this? A fix for the test-suite? |
I think so. Besides, I could merge a workaround PR to replace |
I don't think the test suite issue is from my side, try to run the test suite on just the master branch of this repo and see if it exhibits the same behavior, I'm pretty sure it does. |
It seems that #263 the cookie dep causes the hang |
I think before I go any deeper into this: I'm happy to do a codereview here, if my style of code that I tend to strive for and find most readable works for y'all. As in, I am very thankful you took the time to implement this Ras, I'd just love to split a few things out for example to have a few smaller procs that do individual tasks like a "parseMediaType" proc that returns a parsed media-type and media-type start- and end-indexes etc. However, if that looks like it does more work than it's worth I'm happy to drop that. |
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.
Nice work!
Overall, LGTM. A couple suggestions that you can heed if you want to, I'll leave that up to you. |
Thanks for the code review! |
This fixes #257.
Added a spec-compliant (as far as I can tell) header parser for the Content-Type header, specifically in regards to the
boundary
param. Also added some tests for the new parser andform.nim
.