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

Implement withQueryParams and withMultiValueQueryParams #3024

Merged
merged 2 commits into from Jan 21, 2020
Merged

Implement withQueryParams and withMultiValueQueryParams #3024

merged 2 commits into from Jan 21, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 15, 2019

I could not find a more elegant way of returning Self (and folding on self) other than the proposed

replaceQuery(Query.fromVector(s.withQueryParam(k, v).query.toVector))

If anyone has input on this, let me know.

@ghost
Copy link
Author

@ghost ghost commented Dec 22, 2019

The test failure in the CI appears to be another one of those spotty Blaze tests.

@danielvasilev
Copy link

@danielvasilev danielvasilev commented Jan 7, 2020

@ScalaFanatic do you think it would be beneficial to have a couple of specs for this?

@ghost
Copy link
Author

@ghost ghost commented Jan 8, 2020

@ScalaFanatic do you think it would be beneficial to have a couple of specs for this?

Actually, I considered that, but was unsure how to go about implementing them because QueryOps is a trait. I did not find tests for any other functions in QueryOps, so I just decided to skip it. I could look into adding specs if there is formal guidance on the methodology.

@rossabaker rossabaker added this to the 0.21.0-RC1 milestone Jan 18, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 18, 2020

This one fell victim to my stack-based approach to my GitHub notifications instead of queue-based. Sorry. Milestoned so it's properly considered for the RC cycle this weekend.

Copy link
Member

@rossabaker rossabaker left a comment

This looks reasonable to me, though I agree that I'd like to see a spec. Testing against one of the concrete implementations of QueryOps, like Uri, should be sufficient.

Note: I have barely used our query parameter support, so people who do are encouraged to review.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Would love a test.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 21, 2020

Spec debt captured in #3090.

@rossabaker rossabaker merged commit a97a153 into http4s:master Jan 21, 2020
2 checks passed
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