search

purescript-contrib / purescript-profunctor-lenses

Pure profunctor lenses
MIT License
144 stars 52 forks source link

Update to v0.14.0-rc3 #122

Closed JordanMartinez closed 3 years ago

JordanMartinez commented 3 years ago

Backlinking to purescript-contrib/governance#35

JordanMartinez commented 3 years ago

@kl0tl I'd like your suggestions on role annotations for this PR. Perhaps you could submit a PR after this one?

kl0tl commented 3 years ago

Role annotations are only necessary to relax the nominal roles of foreign data types or to restrict the roles of phantom parameters when they should satisfy some implicit invariant.

There’s no such types here: Forget and Tagged have phantom parameters but as far as I know there’s no invariant to preserve by restricting their roles.

I have audited the purescript-contrib org again to confirm but all the types benefiting from more precise roles have pull requests already:

JordanMartinez commented 3 years ago

Ok. Thanks for clarifying that. I'll post your comment in the meta issue to track that.

kl0tl commented 3 years ago

There’s some usages of SProxy to replace by Proxy or polymorphic type variables in Data.Lens.Record and Test.Main.

JordanMartinez commented 3 years ago

Fixed. Thanks for the reminder!

MonoidMusician commented 3 years ago

I don't feel strongly about it, but it does feel a little weird to me to have kind-polymorphic definitions for things like Getter, when they are never used with a kind other than Type and I don't foresee a use case for them with other kinds. Maybe for individual profunctors like Forget and Tagged it would be useful (for instance, Tagged :: Boolean -> Type -> Type might be useful).

My main concern is that being polymorphic might lead to harder-to-interpret errors in some cases, but I don't know (I haven't tried out kind polymorphism yet).

JordanMartinez commented 3 years ago

I just used the suggestion from the compiler to get this in as fast as possible. I'm not familiar with these types' usages. So, if they can only be used with Type, I say we fix them to that kind.

JordanMartinez commented 3 years ago

My main concern is that being polymorphic might lead to harder-to-interpret errors in some cases, but I don't know (I haven't tried out kind polymorphism yet).

I've pushed a commit that undoes that. Can I get another review?

MonoidMusician commented 3 years ago

Yes, that looks great to me :)

JordanMartinez commented 3 years ago

I'll count that as an approval to this PR then.