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

Add status Methods Which Use String or URI #2518

Merged

Conversation

isomarcte
Copy link
Member

@isomarcte isomarcte commented Apr 18, 2019

These are similar to the expect methods which take the same arguments. They are particularly useful for test.

@isomarcte isomarcte force-pushed the status-from-uri-or-string branch from 9de2264 to 2fec6e0 Compare Apr 18, 2019
@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Apr 18, 2019

I don't think the travis failure is related to my change.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 19, 2019

Thanks. I agree on the CI and restarted Travis.

Whether we want to do this, or move in the opposite direction, is #2483. We should agree on our strategy there, and then either approve or close this.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

I'm for adding them for consistency now, but us all keeping in mind they may be removed with the other overrides sometime in the future.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 20, 2019

If we do these, then also expectOption, expectOptionOr, etc., right? I guess I'm arguing for "all-or-none" rather than "more".

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 22, 2019

It's a lot easier to add methods than take them away, and we have a general aversion to overloads. I think this is a symptom of friction in creating requests. I'd like to think about how we can do better there.

I vote we think about this during 0.20, since 0.21 is likely just around the corner.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Jul 11, 2019

@rossabaker any status updates on this?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jul 16, 2019

I hoped someone would come up with a simpler way of building requests that reduces the desire for overloads, but we haven't seen one yet. I'm usually opposed to overloads, but I guess I'm a mild 👍 on this, since someone cared enough to add it, and nobody has cared enough to shout it down.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Aug 27, 2019

@rossabaker would you prefer statusFromString and statusFromUri? I'm also not a fan of overloads. The only reason I went with that is because that was the idiom I saw in use already.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 28, 2019

Yes, I'd be a lot more in favor of that. I'd also make clear that they're GET requests in the scaladoc.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Aug 28, 2019

@rossabaker will do.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Aug 28, 2019

@rossabaker updated.

These are similar to the `expect` methods which take the same arguments. They are particularly useful for test.
@isomarcte isomarcte force-pushed the status-from-uri-or-string branch from 2fec6e0 to fadaf57 Compare Aug 28, 2019
@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Aug 28, 2019

@rossabaker oops, forgot to push. Updated now.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Reapproving to bump visibility

hamnis
hamnis approved these changes Nov 29, 2019
@ChristopherDavenport ChristopherDavenport merged commit ab1f8d7 into http4s:master Jan 14, 2020
2 checks passed
@isomarcte isomarcte deleted the status-from-uri-or-string branch Jan 21, 2020
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

4 participants