Add ppx_compare to provide comparison functions for Response.t

[msaffer-js]

No description provided.

[mseri]

Thanks! You need to add a dependency to stdlib-shims (I think) for the int and bool compare functions in older compilers

[mseri]

Sorry, it is stdcompat (https://github.com/thierry-martinez/stdcompat) that provides backward compativly the new modules. Then you need to add it to the libraries section in the dune files and replace the comparisons wit( Stdcompat.Bool.compare and Stdcompat.Int64.compare.

May I ask why you want to compare responses? It seems faster doing it by hand and more selectively case by case

[msaffer-js]

First, thanks for the help with this PR!

I'm happy to explain. I'm not sure what you mean when you say it would be faster to do it "by hand" though - could you elaborate?

There are several places where we have a http server and want to make sure that the response that it gives is equal to what we expect; we use compare for this purpose. I can provide some other use cases as well if you'd like. In that case we want to compare the whole thing (headers and all). ppx_compare should also be pretty fast with respect to runtime.

[avsm]

I don't mind adding this, except that it modifies the signature of Response.t and so needs a major bump. While we're here, any reason not to add comparison functions to other modules such as the Request and so on?

[mseri]

@msaffer-js we will have a major bump once conduit 3.0 will be released. Would you mind to rebase this PR once that happens (I'll ping you here) and add the comparison to the other modules (e.g. Request) as mentioned by @avsm?

[msaffer-js]

Sure, I'm happy to do that.

…

On Sat, Jun 27, 2020 at 10:10 AM Marcello Seri ***@***.***> wrote: @msaffer-js <https://github.com/msaffer-js> we will have a major bump once conduit 3.0 will be released. Would you mind to rebase this PR once that happens (I'll ping you here) and add the comparison to the other modules (e.g. Request) as mentioned by @avsm <https://github.com/avsm>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#686 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOGOYPEZTTLIYZ3HATXIOLTRYX4TZANCNFSM4KDKS6GQ> .

[mseri]

@msaffer-js if you are still interested and rebase this PR on master adding the comparison on the other modules as suggested by @avsm, I will be happy to have another look and eventually merge it

[msaffer-js]

Sounds good to me! Is there a merge window that I should be trying to make?

…

On Wed, Oct 21, 2020 at 1:41 PM Marcello Seri ***@***.***> wrote: @msaffer-js <https://github.com/msaffer-js> if you are still interested and rebase this PR on master adding the comparison on the other modules as suggested by @avsm <https://github.com/avsm>, I will be happy to have another look and eventually merge it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#686 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOGOYPHCE7NU4DAMPXDTL4DSL4MNDANCNFSM4KDKS6GQ> .

[mseri]

I aim to make a release sometimes by the end of next week, would you have time to do it before then?

[msaffer-js]

I will give it a shot tomorrow.

…

On Thu, Oct 22, 2020 at 2:45 AM Marcello Seri ***@***.***> wrote: I aim to make a release sometimes by the end of next week, would you have time to do it before then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#686 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOGOYPBZBDO2ULXXHRITR5DSL7IJXANCNFSM4KDKS6GQ> .

[mseri]

@msaffer-js do you have any update on this? Do you need help for some part of it?

[dinosaure]

Sorry @msaffer-js, I took the lead on this PR. The PR is cleaned and I added a ppx_compare on Request.t as requested by @avsm. Should be good to merge 👍.

[mseri]

Looks Good, thanks!