Wrap QueryParams parameters in Maybe

eskimor / servant-purescript

Translate servant API to purescript code, with the help of purescript-bridge.

BSD 3-Clause "New" or "Revised" License

105 stars 44 forks source link

Closed mpodlasin closed 7 years ago

mpodlasin commented 7 years ago

I tested on my project and it works for single QueryParam values.

I did not test yet with QueryParams (Array of parameters) and with code with no query params at all.

mpodlasin commented 7 years ago

Also, shouldn't we leave code generated by tests in repo? That way we can see easily in pull request diffs how exactly the output will change.

mpodlasin commented 7 years ago

Ah, QueryParams (Array) shouldn't be wrapped in Maybe - it's equivalent to having an empty array - I will fix that.

mpodlasin commented 7 years ago

@eskimor Gentle ping. Will you review? This works for me very well.

eskimor commented 7 years ago

friendly reminder, greatly appreciated! I was under the impression that you wanted to fix something first. I will have a look tomorrow!

eskimor commented 7 years ago

Ok, it looks good! But could expand the "test suite" to handle query params so the implementation can be checked more easily? Thank you!

mpodlasin commented 7 years ago

Could you explain a bit what you want me to do? In test/Spec there are already QueryParam used?

eskimor commented 7 years ago

uuuuhm yeah. My fault, sorry about that. :-)

eskimor commented 7 years ago

Good work, thank you!

mpodlasin commented 7 years ago

Thanks for quick response! ;)