Added Ordering instance to HttpDate class

[san-coding]

Contributing to #5017

Added Ordering instance to HttpDate , in branch series/0.22

[san-coding]

@rossabaker , have added Ordering instance to HttpDate , as expected the format tests are failing

[san-coding]

@rossabaker , it is surprising that formatting checks have passed (I never ran sbt scalafmtAll ,
just removed unnecessary spaces ) , but compile checks are also failing, I did what you suggested,

  implicit val stdLibOrderingInstance: Ordering[HttpDate] =  Order[HttpDate].toOrdering

But it is showing error :

[error] -- [E006] Not Found Error: /home/runner/work/http4s/http4s/core/src/main/scala/org/http4s/HttpDate.scala:57:60
[error] 57 | implicit val stdLibOrderingInstance: Ordering[HttpDate] = Order[HttpDate].toOrdering
[error] | ^^^^^
[error] | Not found: Order

Is it because there is no import cats.kernel.Order

[san-coding]

@rossabaker I added import cats.kernel.Order , but now i am getting the following error :

/home/runner/work/http4s/http4s/dsl/src/main/scala-3/org/http4s/dsl/impl/Auth.scala:23: error: ; expected but def found
infix def unapply[F[_], A](ar: AuthedRequest[F, A]): Option[(Request[F], A)] =
^
[error] -- Error: /home/runner/work/http4s/http4s/core/src/main/scala/org/http4s/HttpDate.scala:58:75
[error] 58 | implicit val stdLibOrderingInstance: Ordering[HttpDate] = Order[HttpDate].toOrdering
[error] | ^
[error] |no implicit argument of type cats.kernel.Order[org.http4s.HttpDate] was found for parameter ev of method apply in object Order
[error] one error found
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed

Does it mean that I have to add it as a function like this ?

http4s/core/shared/src/main/scala/org/http4s/ResponsePrelude.scala

Lines 105 to 106 in a942a83

	implicit def stdLibOrdering: Ordering[ResponsePrelude] =
	catsHashAndOrderForResponsePrelude.toOrdering

[san-coding]

I made the change but once again the formatting checks are failing. , the compiles checks are also failing with the following errors

[info] compiling 174 Scala sources to /home/runner/work/http4s/http4s/core/target/scala-3.0.1/classes ...
[error] -- Error: /home/runner/work/http4s/http4s/core/src/main/scala/org/http4s/HttpDate.scala:59:19
[error] 59 | Order[HttpDate].toOrdering
[error] | ^
[error] |no implicit argument of type cats.kernel.Order[org.http4s.HttpDate] was found for parameter ev of method apply in object Order
[error] one error found
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 47 s, completed Sep 13, 2021 10:04:58 AM

@isomarcte @rossabaker , looks like I am at a dead-end, what do I do now

[rossabaker]

My mistake. I thought we had a cats.kernel.Order instance for this. That's a lot more interesting than the standard library one, which can be implicitly derived from the Cats one. Let's try this. Do you have a working compiler yet?

Add this to HttpDate instead of what you have:

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {

    }

You'll need to add an import of cats.Order. The compiler will tell you what method is missing. See if you can add it and implemented. When that works, we can discuss testing.

[san-coding]

My mistake. I thought we had a cats.kernel.Order instance for this. That's a lot more interesting than the standard library one, which can be implicitly derived from the Cats one. Let's try this. Do you have a working compiler yet?

Add this to HttpDate instead of what you have:

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {

    }

You'll need to add an import of cats.Order. The compiler will tell you what method is missing. See if you can add it and implemented. When that works, we can discuss testing.

Alright thanks @rossabaker , I will look into it , but @isomarcte had suggested a method to implement Ordering instance even when we don't have cats.kernel.Order instance
This is what he had suggested
#5017 (comment)

I tried this with the Ordering definition that you suggested , but that was giving error,

Now I will try implementing cats.kernel.Order instance as you have suggested

[rossabaker]

Yes, what he suggested should work. I got you in trouble because I mistook that we already had the Cats instance.

[san-coding]

Hey @rossabaker , I have also overridden compare method, is that needed? what all should inside the catsOrderForHttp4sHttpDate instance {}

[san-coding]

Looks like Compile check are failing because

value headerName is not a member of org.http4s.HttpDate

[san-coding]

So, what should I put inside instance, usually I have seen the compare function, but I added compare and it gave the above error

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {

    }

[isomarcte]

@san-coding if you are going to add add an Order can you also add a Hash with it? Something like we do here https://github.com/http4s/http4s/blob/main/core/shared/src/main/scala/org/http4s/RequestPrelude.scala#L102

Sorry, this is extra confusing and weird. The reason we have to do this is that both Order and Hash extend Eq and if they aren't both in the concrete instance we get into an ambiguous implicit context.

So, if we don't add a Hash instance at the same time we add an Order we'll have to do a deprecation in the future if we ever want to add a Hash.

I hope that makes sense....I'm sorry, it's very confusing.

[san-coding]

@isomarcte @rossabaker , Ok I will add Hash, but can you tell me what I should put inside the instance definition

 implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {
    What should add here
    }

[san-coding]

Hey @rossabaker @isomarcte , QValue class is extending Ordered, and has a cats.kernel instance, but and Ordering instance has to be added to it right?

http4s/core/shared/src/main/scala/org/http4s/QValue.scala

Line 131 in a942a83

implicit val catsInstancesForHttp4sQValue: Order[QValue]

I think HttpVersion class also needs Ordering instance

http4s/core/shared/src/main/scala/org/http4s/HttpVersion.scala

Lines 30 to 32 in a942a83

	final case class HttpVersion private[HttpVersion] (major: Int, minor: Int)
	extends Renderable
	with Ordered[HttpVersion] {

ContentCoding also requires Ordering instance

http4s/core/shared/src/main/scala/org/http4s/ContentCoding.scala

Lines 37 to 40 in a942a83

	class ContentCoding private (val coding: String, override val qValue: QValue = QValue.One)
	extends HasQValue
	with Ordered[ContentCoding]
	with Renderable {

There are many more classes which require Ordering instance, I will search and make PRs for each of them one by one

[san-coding]

@rossabaker @isomarcte
When I had added def compare() definition inside the instance :

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {
      override def compare(x: HttpDate, y: HttpDate): Int =
        x.headerName.compareTo(y.headerName)
    }

It gave this error :

value headerName is not a member of org.http4s.HttpDate

Then I removed the def compare() and left it empty like this

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {
    }

Now it says object cannot be created because def compare() is missing

[error] -- Error: /home/runner/work/http4s/http4s/core/src/main/scala/org/http4s/HttpDate.scala:59:4
31
[error] 59 | new Order[HttpDate] {
32
[error] | ^
33
[error] |object creation impossible, since def compare(x: A, y: A): Int in trait Order in package cats.kernel is not defined

Should it be x.epochSecond.compare(y.epochSecond)

[san-coding]

Hurray 🎉, finally it is compiling , so this is the cats instance definition I added

  implicit val catsOrderForHttp4sHttpDate: Order[HttpDate] =
    new Order[HttpDate] {
      override def compare(x: HttpDate, y: HttpDate): Int =
        x.headerName.compareTo(y.headerName)
    }

@isomarcte @rossabaker , only formatting checks are failing ,I will add Hash soon, please review and give your feedback (@hamnis please help out with sbt scalafmtAll commit)

[san-coding]

Hey @isomarcte @rossabaker , I have made the changes you requested: added Hash ,compile checks have passed , only formatting checks are failing, I think this PR is ready for review once someone helps out with sbt scalafmtAll commit @hamnis , thanks

[rossabaker]

Do you have sbt working yet? Or Metals? Either of those should be able to do the formatting for you.

I took care of this one.

[san-coding]

Thanks @rossabaker @isomarcte , should I make anymore changes or is this PR ready to be merged

[san-coding]

Which classes should I next add Ordering instance

[san-coding]

Do you have sbt working yet? Or Metals? Either of those should be able to do the formatting for you.

I took care of this one.

I have installed metals, but set doesn't seem to be working

[san-coding]

Do you have sbt working yet? Or Metals? Either of those should be able to do the formatting for you.

I took care of this one.

I think I have figured it out on VSCode 🎉

[rossabaker]

This should have tests, too. Look at HttpVersionSuite for how it has checkAll on OrderTests and HashTests. That's how we know that the instances you created are lawful.

[san-coding]

This should have tests, too. Look at HttpVersionSuite for how it has checkAll on OrderTests and HashTests. That's how we know that the instances you created are lawful.

Ohh okay, I will make the required changes @rossabaker , thanks

[san-coding]

This should have tests, too. Look at HttpVersionSuite for how it has checkAll on OrderTests and HashTests. That's how we know that the instances you created are lawful.

You mean I have to add a OrderingTests , should I make a HttpDateSuite.scala ? I am a little confused here, @isomarcte @rossabaker

EDIT: just noticed that HttpDateSuite.scala already exists on branch a942a83e2b but not on series/0.22, what do I do now

should I create that file in branch series//0.22 and add required tests to it ?

@rossabaker which branch should I be working on in the future

[rossabaker]

Adding tests like this to the existing HttpDateSuite on this branch should get the job done. You will probably need to add imports: see the linked file for an example.

[san-coding]

Adding tests like this to the existing HttpDateSuite on this branch should get the job done. You will probably need to add imports: see the linked file for an example.

Ok , I will add the HttpDateSuite.scala to branch series/0.22

[san-coding]

@rossabaker I have added test and made the requested changes

[rossabaker]

👍 if green

[san-coding]

@rossabaker Thanks, is this ready to be merged ? What should I work on next @rossabaker , want to contribute more, which files need Ordering instance @rossabaker @isomarcte

[rossabaker]

if you can search for files that have Ordered and don't have a Cats Order, that's great: the Cats instances are what are the most interesting.

[san-coding]

if you can search for files that have Ordered and don't have a Cats Order, that's great: the Cats instances are what are the most interesting.

Thanks , will start working on it immediately 🎉