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 Hash and BoundedEnumerable instances for HttpVersion #3902

Merged
merged 5 commits into from Nov 25, 2020

Conversation

ammarc
Copy link
Contributor

@ammarc ammarc commented Nov 22, 2020

For #3869

Test had to be adapted in order to fix the following compile error:

ambiguous implicit values:
 both value http4sHttpOrderForVersion in object HttpVersion of type cats.Order[org.http4s.HttpVersion]
 and value http4sHttpHashForVersion in object HttpVersion of type cats.Hash[org.http4s.HttpVersion]
 match expected type cats.kernel.Eq[org.http4s.HttpVersion]
  checkAll("HttpVersion", OrderTests[HttpVersion].order)

Do let me know if there's a better way to fix this.

@rossabaker
Copy link
Member

rossabaker commented Nov 22, 2020

Thanks for volunteering!

The problem is that Hash and Order both extend Eq, so now we have two implicit Eq instances at the same priority, when we want exactly one. I didn't think about this until now. The problem we're having in the tests will also happen downstream, so we ought to fix this.

One way around it is to make one instance that has all the Cats instances. Case-insensitive has an example. If you copy and paste from that to get all the methods, this can't be a Monoid, but it could be a BoundedEnumerable instead of a LowerBounded.

We would need to remove the implicit keyword from the existing instances, and should add a @deprecated annotation. This is all binary compatible, so the deprecation version could be "0.21.12" and we need to target series/0.21.

Whew, that turned into a lot. If not all of that makes sense, please ask! 😄

@ammarc
Copy link
Contributor Author

ammarc commented Nov 23, 2020

Thanks for the explanation, Ross.

Why do we need to add BoundedEnumerable? Is it because that adds more information to HttpVersion and is a better representation of this type?

@ammarc ammarc changed the base branch from main to series/0.21 November 23, 2020 03:50
@ammarc ammarc changed the base branch from series/0.21 to main November 23, 2020 03:52
@ammarc
Copy link
Contributor Author

ammarc commented Nov 23, 2020

we need to target series/0.21

Do you mean I need to make this PR to http4s:series/0.21 iso. http4s:main?

@ammarc ammarc changed the base branch from main to series/0.21 November 23, 2020 04:35
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done. One more thing. Whenever we add an instance, we like to check its laws. So in HttpVersionSpec, we should add:

  checkAll("HttpVersion", HashTests[HttpVersion].hash)
  checkAll("HttpVersion", BoundedEnumerableTests[HttpVersion].boundedEnumerable)

And with that, you should see a failing test on BoundedEnumerable. 1.0, 1.1, and 2.0 are the common ones, but there are actually 100 valid ones.

I know this is more than you signed up for, and really appreciate the work on this!

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@rossabaker rossabaker added this to the 0.21.12 milestone Nov 24, 2020
@rossabaker
Copy link
Member

CI didn't run due to #3916, but I verified everything locally.

@rossabaker rossabaker changed the title add Hash instance for HttpVersion Add Hash and BoundedEnumerable instances for HttpVersion Nov 24, 2020
@rossabaker rossabaker merged commit f170139 into http4s:series/0.21 Nov 25, 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

2 participants