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

Support flag query parameters in DSL #2266

Merged
merged 3 commits into from Nov 13, 2018
Merged

Support flag query parameters in DSL #2266

merged 3 commits into from Nov 13, 2018

Conversation

@howyp
Copy link
Contributor

@howyp howyp commented Nov 11, 2018

Adds an new matcher to the DSL supporting option flag-style query parameters as discussed in #2254.

@howyp howyp changed the title WIP - Support flag query parameters in DSL Support flag query parameters in DSL Nov 11, 2018
@howyp
Copy link
Contributor Author

@howyp howyp commented Nov 11, 2018

Fixes #2254

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 11, 2018

I shut down the appveyor build, so don't worry about that failure.

@aeons
aeons approved these changes Nov 12, 2018
}
"optional flag parameter when present with a value" in {
val response =
serve(Request(GET, Uri(path = "/flagparam", query = Query.fromString("flag=1"))))

This comment has been minimized.

@rossabaker

rossabaker Nov 12, 2018
Member

There's no desire to implement "truthiness" here, correct? This would still pass if flag=0? I think it should, and does, but just double checking the intent.

This comment has been minimized.

@howyp

howyp Nov 13, 2018
Author Contributor

Yes that was my intent. I was more trying to show that true is returned whether there is a value or not - the value is ignored, all that matters is if the parameter key is there or not. I guess I could express that better in this test.

I actually wavered a bit on whether boolean was the right return type. WDYT? The alternative was adding a little ADT with Present and Absent objects, which might be a bit clearer.

Or ... we could have some arrangement where you get a different type if:

  1. The parameter is completely absent
  2. The parameter is present but with no value
  3. The parameter is present but with a value (and then I guess you'd want the capabilities we have elsewhere for decoding the value)

But IMHO that seemed a bit overcomplicated? I'm not sure if anyone would actually use parameters in this way.

This comment has been minimized.

@rossabaker

rossabaker Nov 13, 2018
Member

Present and Absent would make clear that it has no opinions on truthiness, but would generally be a bit more inconvenient to use.

I share your uncertainty on the ternary extractor. It looks good on paper but I'm not sure about on practice.

I don't deal with APIs that use query parameters often, so I don't have a lot of practical experience here. This is merged, and it made things better. Let's try it, and if it gives us a rash, we can fix it before 1.0.

@aeons aeons merged commit ec77e6b into http4s:master Nov 13, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@howyp howyp deleted the howyp:flag-params branch Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.