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 cats Eq/Order for Quantity #95

Merged
merged 6 commits into from
Jul 20, 2020

Conversation

cquiroz
Copy link

@cquiroz cquiroz commented Jul 17, 2020

This is a first attempt at a coulomb-cats module. It contains now just the definitions of Eq and Order for Quantity[A, U]. Hopefully it can be expanded to get us Functor but I had some troubles on my attempts

It contains two modules: coulomb-cats contains the typeclasses and coulomb-testkit that contains Arbitrary and Cogen instances for test and discipline tests. If you prefer I could move the tests to coulomb-tests

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

Adds coulomb-cats

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@erikerlandson
Copy link
Owner

This looks good. One last nit is that the file name .../cats/package.scala is typically convention for the file defining a package-object. I'd name this file cats/implicits.scala

@erikerlandson
Copy link
Owner

Oh, can you add a file coulomb-cats/README.md? An example would be coulomb-refined/README.md - some information on how to include in project, maybe a couple short code examples

@cquiroz
Copy link
Author

cquiroz commented Jul 19, 2020

Just now that I'm writing the documentation I realize that Quantity already has a === operator. It makes me wonder if we really need Eq instances, though there are other uses of course like when using scalacheck

@erikerlandson
Copy link
Owner

I think there are potential use cases. I wanted "unit aware" comparisons but something like sorting an array is likely to be a case of comparing a collection of quantities having all the same value and unit types.

Changed package name, added documentation and more tests

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@cquiroz
Copy link
Author

cquiroz commented Jul 20, 2020

I added a readme and a few more test cases.
I move the package from coloumb.cats.implicits to coloumb.implicits. This is because often you import coloumb._ and the cats package would shadow the root cats package. Let me know if that seems ok

@erikerlandson
Copy link
Owner

This is because often you import coloumb._ and the cats package would shadow the root cats package.

That is a very interesting point. I have to assume this comes up for all of my integrations, as I have coulomb.refined, coulomb.pureconfig, etc. Currently I think this would be resolved via something like import _root_.pureconfig..., for example here.

In the immediate situation, what I propose is that we stick with coulomb.cats.implicits, and I suppose library users would then need import _root_.cats.implicits._ which is a bit of a hack but at least it is consistent with the rest of the coulomb integration packages.

In the longer term (a future coulomb-0.6 ?), this makes me want to re-think the package paths I am declaring, which would of course break some backward compatibility, but maybe some convention like import coulomb.integrations.cats... or perhaps invert it, like import cats.coulomb....

@cquiroz
Copy link
Author

cquiroz commented Jul 20, 2020

Ok it makes sense. It is a problem mostly if you try to import something from cats after coulomb

@cquiroz
Copy link
Author

cquiroz commented Jul 20, 2020

I'll update it

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@cquiroz
Copy link
Author

cquiroz commented Jul 20, 2020

I moved the package back as suggested

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
@erikerlandson
Copy link
Owner

LGTM, Thanks @cquiroz!

@erikerlandson erikerlandson merged commit 2b8f100 into erikerlandson:develop Jul 20, 2020
@erikerlandson erikerlandson mentioned this pull request Jul 20, 2020
cquiroz added a commit to cquiroz/coulomb that referenced this pull request Jul 22, 2020
* Add cats Eq/Order for Quantity

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

Adds coulomb-cats

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

* Updated according to PR comments

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

* More suggested changes from PR

Changed package name, added documentation and more tests

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

* Moved implicits package to coulomb.cats

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

* Fix reference to package name in the README

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>

* Remove documentation typo

Signed-off-by: Carlos Quiroz <carlos.m.quiroz@gmail.com>
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