purescript-deprecated/purescript-generics-rep

[generics-rep] Add instances for some usual algebraic types?

Closed this issue · 25 comments

kindaro commented

As I understand from documentation, instances for Generic may only be defined in either:

  • The module that defines the subject type.
  • The module that defines the class Generic.
  • The module that defines the type used to construct the generic representation.

In the case of Generic, the latter two coincide in Data.Generic.Rep.

This means that I cannot possibly define an instance of Generic for any type I do not define myself in the first place. This includes all the usual types like Unit or Either. The only instance provided by this package is for Maybe — this is not enough. I propose that more instances be given.

Since generics are such a basic thing, I further propose that the right place to define instances is in the modules that define subject types. This includes the Maybe instance — it should be moved away for consistency.

garyb commented

The dependency for Maybe can't be reversed as this library uses it in the implementation of GenericEnum.

garyb commented

Although perhaps GenericEnum could be provided in purescript-enums instead too, it might well work out then.

kindaro commented

@garyb  Am I to understand that you are in favour of this undertaking in principle?

paulyoung commented

#34 (comment) says:

The instance for Either should live in purescript-either instead, and should be derivable.

So I did that in purescript/purescript-either#57

garyb commented

@kindaro yeah, it seems reasonable to me - since Generic has compiler support it's pretty special in the ecosystem anyway, so moving it slightly closer to the root of the dependency hierarchy isn't a problem in my book.

Deriving the instance in -maybe rather than having to hand write it the way it is just now will be nice too.

I think if we were to move the GenericEnum stuff it should probably go as Data.Enum.Generic. That pattern is the usual way it works for situations where generic helpers are provided outside of this library. Actually, there's an argument that this pattern should be used in all cases for consistency. This library could then have no dependencies, and either be merged into Prelude, or -prelude could become a dependent and the classes moved there. That's a bit more of a controversial stance perhaps though, would be interesting to see if other core team members have thoughts. @hdgarrood might be in favour, since he's expressed interest in having a larger single Prelude already.

hdgarrood commented

I like the idea of using the same pattern in all cases, and I also think moving Generic closer to the root of the dependency hierarchy makes sense, for the same reason. I would be in favour of merging this library into prelude, assuming we don't plan to re-export any of it from the Prelude module.

garyb commented

Yeah, that was what I was thinking - just changing where it resides, not the Prelude module.

  • 👍1
kl0tl commented

Is this something we should do in 0.14?

JordanMartinez commented

I think so.

hdgarrood commented

Unless this is likely to be breaking in some way (I don’t think it is?), then I think we should consider postponing it so that we can get the release out sooner.

  • 👍1
JordanMartinez commented

Unless this is likely to be breaking in some way (I don’t think it is?)

So, here's my understanding of what we'd have to change:

  • move this repo (excluding the GenericEnum module) into the purescript-prelude repo. However, we do not re-export its contents from the Prelude module. The modules names stay the same.
  • move this repo's GenericEnum module, which depends on maybe into purescript-enums. purescript-enums already depends on maybe, so this isn't a breaking change there.
  • Add instances to other repos (e.g. Unit in prelude; either in Either; Maybe in maybe, etc.)

The only breaking changes we might make are the names of the modules. Quoting Gary:

I think if we were to move the GenericEnum stuff it should probably go as Data.Enum.Generic. That pattern is the usual way it works for situations where generic helpers are provided outside of this library. Actually, there's an argument that this pattern should be used in all cases for consistency.

and Harry:

I like the idea of using the same pattern in all cases

Renaming the modules would be a breaking change. If we do not rename them, we could migrate the modules after v0.14.0. If we rename them, we should do it before v0.14.0. Which one should we take?

I believe the modules would be renamed to the following:

  • Data.Generic.Rep.Bounded -> Data.Bounded.Generic
  • Data.Generic.Rep.Enum -> Data.Enum.Generic
  • Data.Generic.Rep.Eq -> Data.Eq.Generic
  • Data.Generic.Rep.HeytingAlgebra -> - Data.HeytingAlgebra.Generic
  • Data.Generic.Rep.Monoid -> Data.Monoid.Generic
  • Data.Generic.Rep.Ord -> Data.Ord.Generic
  • Data.Generic.Rep.Ring -> Data.Ring.Generic
  • Data.Generic.Rep.Semigroup -> Data.Semigroup.Generic
  • Data.Generic.Rep.Semiring -> Data.Semiring.Generic
  • Data.Generic.Rep.Show -> Data.Show.Generic
hdgarrood commented

Yes, good point, thanks. I’m not sure why I thought this wouldn’t be breaking. I agree then, now probably is a good time to do it.

hdgarrood commented

Unit shouldn’t get a Generic instance, incidentally. We should only provide Generic instances when a data type’s constructors are exported. But other than that this all sounds good 👍

I think doing it now and renaming the modules sounds preferable. Moving a module from one library to another is always sort of breaking anyway, because you’ll end up with duplicate module errors if you don’t drop the library where those modules previously lived from your dependencies.

JordanMartinez commented

Things left to do:

Anything else?

JordanMartinez commented

All other PRs pass CI and are ready for review. See above comment for links to them.

JordanMartinez commented

All PRs have been merged. The only thing left to do here is transfer the issues and deprecate this repo.

kindaro commented

Woo-hoo, such progress in such a short time. Thank you Jordan and allies.

JordanMartinez commented

I renamed all issues in this repo, so that they include "[generics-rep] " in front of the original issue. I've transferred all open issues in this repo to purescript-prelude. Not sure whether I should also transfer this one.

There's two pull requests currently open. I'm guessing we should close them and open issues in prelude for them if they are still desired.

  • ❤️1
JordanMartinez commented

Ok. All PRs have been addressed:

So, I believe the only thing left to do is deprecate this repo.

JordanMartinez commented

As stated above, I believe the only thing left to do is to deprecate this repo. Does someone need to transfer this repo to the purescript-deprecated organization and update its Readme to clarify why it was deprecated?

thomashoneyman commented

I haven’t been following closely enough to judge if everything else is ready to go, but I can take care of deprecating this library if so.

thomashoneyman commented

I've moved this library to the deprecated organization. So long as no one remembers something else we need to do (@hdgarrood, @kl0tl?), I can complete the deprecation process tomorrow night and upload the notice to Pursuit.

hdgarrood commented

I think it might be best to wait until after 0.14.0 is released to tell Pursuit that this library has been deprecated, because we want the current version to still appear in search results until then.

thomashoneyman commented

That's a good reason to wait; I'll leave this issue open.