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

Document optional params #6

Merged
merged 4 commits into from
Mar 6, 2022
Merged

Document optional params #6

merged 4 commits into from
Mar 6, 2022

Conversation

raquo
Copy link
Contributor

@raquo raquo commented Apr 3, 2021

One of the more popular URL-DSL questions I've seen is how to make optional params so I added a short but explicit line about that in the readme.

I also added some test assertions to check my own understanding. Tests currently fail because the roundtrip does not work for Some("") in case of an optional String param. What I mean is: if you have param[String]("empty").?, and put the value Some("") into it, it will add "empty=" to the URL, which is expected. However, if you try to parse this URL back into the same pattern, you will get None instead of Some("").

Is it possible to fix this, so that such a URL decodes as Some("")?

@raquo
Copy link
Contributor Author

raquo commented Apr 3, 2021

Note: edited description to fix a typos: forgot the .?, mixed up Right("") and Some("")

@sherpal
Copy link
Owner

sherpal commented May 1, 2021

Yes, indeed, it seems like a necessary addition to the docs.

For your question, it should be quite easy to fix, I will try to have a look this week-end/week.

raquo and others added 2 commits May 14, 2021 14:32
- bug: parsing empty query parameter like `http://localhost:8080?foo=`as String  returned None instead of Some("").
- improve: check if query parameter's key is valid. `http://localhost:8080?=value` was parsed into "" -> Param(value).
@i10416
Copy link
Contributor

i10416 commented Dec 31, 2021

Hi, I made a small fix for this. raquo#1

@sherpal sherpal merged commit 134e854 into sherpal:master Mar 6, 2022
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.

3 participants