Improve error messages from FromMultipart

[maksbotan]

Hi,

I've tried using servant-multipart and observed that error messages from parsing are not very informative. Therefore I decided to introduce Lenient mode, analogous to Capture.

[maksbotan]

8.0 and 8.2 builds failed due to <> missing from Prelude (I've removed Data.Monoid import). How about we drop them and raise lower requirement on base to 4.8?

[fisx]

Thanks! Good error messages are useful. I will take a look at this soon.

8.0 and 8.2 builds failed due to <> missing from Prelude (I've removed Data.Monoid import). How about we drop them and raise lower requirement on base to 4.8?

I personally think supporting everything down to 8.0 is not too much to ask. As long as we don't have any evidence that nobody cares, I wouldn't touch it.

Anyway that would be a different PR, no?

[maksbotan]

Okay, I've reverted that small change, now it should build on 8.0.

I have another thought: do we really need the "fromMultipart failed:" prefix in the error message? I suppose it should be OK to drop it and return the parser's message verbatim?

[fisx]

Okay, I've reverted that small change, now it should build on 8.0.

I have another thought: do we really need the "fromMultipart failed:" prefix in the error message? I suppose it should be OK to drop it and return the parser's message verbatim?

The extra prefix could be helpful, depending on how unhelpful the parser is, which depends on the user of the library, so I'm not sure we should depend on it.

Would "Could not decode multipart mime body: <parser error message>" be an improvement? I'd prefer something like this because it does not leak servant internals to the API.

[fisx]

This is really nice! Since it's a breaking change, I would like a second opinion, but I'd be happy to release this. Thanks @maksbotan!

[fisx]

There are ways to make this change easier for the library users, but I'm not sure it should be. Perhaps it's good to force everybody to take a look at this if they can't somehow produce a useful error here; if not, it's still straight-forward enough to do something like maybe (Left "no error info, sorry") Right to the maybe-parser.

Yes, I think it should be a breaking change. @arianvp, @dmjio anybody: any opinions?

[fisx]

Okay, I've reverted that small change, now it should build on 8.0.
I have another thought: do we really need the "fromMultipart failed:" prefix in the error message? I suppose it should be OK to drop it and return the parser's message verbatim?

The extra prefix could be helpful, depending on how unhelpful the parser is, which depends on the user of the library, so I'm not sure we should depend on it.

Would "Could not decode multipart mime body: <parser error message>" be an improvement? I'd prefer something like this because it does not leak servant internals to the API.

Since the PR is such an obvious improvement either way: if you feel strongly about this and nobody else objects, I'm ok with the change you suggest.

[maksbotan]

Hi,

I'm okay with extended message hiding fromMultipart. I'll try to commit it today.

Re breaking changes: I believe there is no need in transition mechanism, let users see this right away and be able to react. Anyway I suppose this will go into major version bump, like 0.12.0.

Re nitpicks: I'll also react to them in a commit today :)

Thanks for the review!

[maksbotan]

I've rebased the history to make it pretty and incorporated your ideas. Thanks!

Just in case: after this is merged, I'm going to make a PR with regenerated haskell-ci and another one with version bump.

[maksbotan]

Ping? :)

[arianvp]

i'll try to give this a quick look tomorrow. if I don't then Matthias just should merge this I think

[fisx]

Thanks again, and sorry for the delay!

Not sure about when we'll make a release. Probably not before zurihac.

[maksbotan]

Thanks!