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

Split into servant-multipart-api, servant-multipart-client, servant-multipart #51

Merged
merged 14 commits into from Jun 5, 2021

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Apr 29, 2021

This is about #48

cc @maksbotan

@tfc tfc changed the title DraftL Split into servant-multipart-api, servant-multipart-client, servant-multipart Draft: Split into servant-multipart-api, servant-multipart-client, servant-multipart Apr 29, 2021
@tfc
Copy link
Contributor Author

tfc commented Apr 29, 2021

I might need a little help deciphering what happened in the CI.

@maksbotan
Copy link
Contributor

Basically Travis should be considered dead. I'll migrate to github actions, hopefully tomorrow.

@tfc
Copy link
Contributor Author

tfc commented May 5, 2021

What is the current state? Is there anything i can/should do to increase chances of progress?

@maksbotan
Copy link
Contributor

Hi, sorry for the delay!

I've updated CI in master and in your branch. Looks like compatibility with ghc <8.4 is broken due to missing <> import. I'm personally in favor of dropping those, but since we are talking about possible move into main servant repo, we should support the same GHC versions servant itself supports. Can you please fix that?

I'll take a more thorough look in the next couple of days.

@tfc
Copy link
Contributor Author

tfc commented May 16, 2021

Can you please fix that?

Should be fixed now.

@tfc
Copy link
Contributor Author

tfc commented May 16, 2021

@maksbotan did you use some kind of tool to determine unused deps or did you determine that by hand?

@maksbotan
Copy link
Contributor

@maksbotan did you use some kind of tool to determine unused deps or did you determine that by hand?

I used this feature of GHC 8.10: https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/using-warnings.html#ghc-flag--Wunused-packages

Copy link
Contributor

@maksbotan maksbotan left a comment

Choose a reason for hiding this comment

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

A couple of minor things, otherwise looks good. Thanks for your work!

instance ToMultipart tag (MultipartData tag) where
toMultipart = id

class MultipartBackend tag where
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this class, along with resourcet dependency, belongs in servant-multipart package, not the -api one. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought the same, but then i am clueless about this:

  • class MultipartBackend defines type MultipartResult
  • type MultipartResult is needed by type FileData
  • type FileData is needed by type MultipartData
  • type MultipartData is needed by classes FromMultipart and ToMultipart
  • and ToMultipart seems like a client thing

if the last bullet point is correct then it seems like moving this all into the servant-multipart package is not the right thing, or is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in the current state of the PR one can't actually use client without server, since client needs MultipartResult tag instance to construct the input.

I propose we make MultipartResult a top-level open type family instead of associated type and define Mem and Tmp instances in this module. Then, move this class to server. Now the client user no longer needs to import server package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds good, but this touches the edge of my haskell knowledge. I'd love to do that, but then i need instructions in a little bit more explicit steps.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean you leave in this module only this:

type family MultipartResult (tag :: *) :: *
type instance MultipartResult Mem = ...
type instance MultipartResult Tmp = ...

The rest goes to the respective server and client packages.

If you have no luck with this, I'll try to do it myself tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is helpful, i will try it. won't have time for that today though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i tried my luck. This is pretty straightforward up to the point where loadFile is used from the servant-multipart-client library but defined in the backend class which i moved to servant-multipart. While trying to move the loadFile function out of the backend class to the client, i somehow got lost in the whole idea around the type proxy mixed with the new type family.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done it, please take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks great, thanks!

@tfc
Copy link
Contributor Author

tfc commented Jun 2, 2021

@maksbotan what is missing here? from my perspective it looks like (due to your help, thank you) we are down to "remove the SourceT mappend". can we maybe do this in a followup-MR and go towards merging this code?

@maksbotan
Copy link
Contributor

Hi @tfc! Actually, we are down to "I need to find time to merge this" :)
I will really try to come back tomorrow, if that does not happen, feel free to ping me on Friday. Sorry!

Copy link
Contributor

@maksbotan maksbotan left a comment

Choose a reason for hiding this comment

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

Nice! I think it's all OK now. I will check my project with this branch, just in case, to see if it's backwards-compatible.

If it's OK, I will merge this soon.

Thanks for your contribution!

@maksbotan maksbotan changed the title Draft: Split into servant-multipart-api, servant-multipart-client, servant-multipart Split into servant-multipart-api, servant-multipart-client, servant-multipart Jun 5, 2021
@maksbotan maksbotan merged commit 909539e into haskell-servant:master Jun 5, 2021
@tfc
Copy link
Contributor Author

tfc commented Jun 5, 2021

Great, thank you for this great open source co-op experience!

@Profpatsch
Copy link

Oh so this is why loadFile in client will always die of a closed handle if I try to stream from it on the server. It only works for the client.

A tiny bit of documentation would have saved me 3 hours just now :(

@Profpatsch
Copy link

That is

Servant.API.Stream.fromSourceIO $ Servant.Multipart.loadFile (Proxy :: Proxy Servant.Multipart.Tmp)

typechecked just fine and gave me a ConduitT () Lazy.ByteString IO (), which I thought I could use to stream the contents of the file on the server side, but I immediately got

/tmp/servant-multipart1919612-15.buf: hGetBuf: illegal operation (handle is closed)

for reasons that I can understand now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants