Skip to content
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

Allow IO when unrendering content types #343

Closed
wants to merge 4 commits into from
Closed

Conversation

fizruk
Copy link
Member

@fizruk fizruk commented Jan 19, 2016

This makes path to multipart/form-data content type, as mentioned in #133 (comment).

@fizruk fizruk force-pushed the fizruk/unrender-io branch 2 times, most recently from 1e222bd to 276dfd8 Compare January 20, 2016 00:55

class AllCTUnrender (list :: [*]) a where
handleCTypeH :: Proxy list
-> ByteString -- Content-Type header
-> ByteString -- Request body
-> Maybe (Either String a)
-> Maybe (ExceptT String IO a)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason we're keeping the Maybe that we want to be able to not do IO?

@alpmestan alpmestan mentioned this pull request Mar 15, 2016
@soenkehahn
Copy link
Contributor

@fizruk: This PR is a bit old and stale. Should probably be rebased on current master.

Also I'm not clear on the motivation. Was this to support file-uploads?

@jkarni
Copy link
Member

jkarni commented Aug 17, 2016

Also I'm not clear on the motivation. Was this to support file-uploads?

Yes.

I think this is still the right approach for that, especially since I think it's easier to do with recent changes. I'll take a look at this PR today, and try to rebase it.

@jkarni jkarni force-pushed the fizruk/unrender-io branch from 276dfd8 to 9d9091f Compare August 17, 2016 17:37
@jkarni
Copy link
Member

jkarni commented Aug 17, 2016

Done. Tests pass and we no longer need unsafeInterleaveIO. That said, I think this needs a more precise set of tests about the functionality in servant-server (i.e., when the file is written, what errors are thrown if that fails, etc.). And the easiest way of writing those tests, to my mind, is writing the multipart content-type. So at the risk of scope creep, I'd wait on merging this.

@jkarni
Copy link
Member

jkarni commented Aug 17, 2016

Taking a further look, it seems like it makes the most sense to keep the new MultiPartFormData combinator as a separate package, since the most natural way to define the instances is to use wai-extra, which is not a dependency I'm happy with servant having.

Another point to note is that I don't immediately see a nice way of having configurable options - e.g. file size limits. If we make this not specific to MultiPartFormData - e.g., if we have the maximum request body size in the Context - it would be nicer.

For now I'm going to use the default configuration options.

@jkarni
Copy link
Member

jkarni commented Aug 18, 2016

For reasons mentioned in #133, I'm in favor of closing this. @fizruk agreed?

@phadej
Copy link
Contributor

phadej commented Jan 21, 2017

ping @fizruk, can this be closed?

@phadej
Copy link
Contributor

phadej commented May 14, 2017

Closing this.

@phadej phadej closed this May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants