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

Migrate away from Eq1 / Ord1 / Show1 #28

Closed
wants to merge 1 commit into from
Closed

Migrate away from Eq1 / Ord1 / Show1 #28

wants to merge 1 commit into from

Conversation

Bodigrim
Copy link

Closes #27.

Tested with

packages: .
packages: dhall-1.42.1
packages: aeson-2.2.1.0
packages: hnix-0.17.0
packages: sexp-grammar-2.3.4.2
packages: recursion-schemes-5.2.2.5
packages: quickcheck-instances-0.3.30
packages: liquidhaskell-boot-0.9.8.1

allow-newer: hnix:aeson, hnix:free, hnix:bytestring, hnix-store-core:algebraic-graphs

I'm on aarch64 so unfortunately cannot test locally with old GHCs.

Really old GHC <= 7.6 fail in CI. If we must retain compatibility with them, I can revert changes with regards to Typeable1.

@Bodigrim
Copy link
Author

Bodigrim commented Mar 23, 2024

@phadej I got access to an x86_64 machine and tested the patch with GHC 8.4 and 8.6. No breakage in downstream packages detected.

@Bodigrim
Copy link
Author

@phadej anything else you would like me to check here?

@phadej
Copy link
Collaborator

phadej commented Apr 20, 2024

Everything looks fine.

I'm waiting for GHC-9.10 release; as this is still a breaking change, I'd like to make a 9.10 compatibility work first, and only then make a major release.

(==) = (==) `on` foldMu Fix

instance (Functor f, Ord1 f) => Ord (Mu f) where
instance (Functor f, Ord (f (Fix f))) => Ord (Mu f) where
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now thinking more about it. I'm not happy about this change. It exposes too much of implementation detail to the surface. Ord1 f is asking for more, but doesn't tell anyone on the outside that Fix is used internally.

I'm not particularly happy with this. So I'm sorry, but we'll need to rethink this change.

Changing Fix interface is one option, but I don't like that either, as then Fix and Mu/Nu parts will look different.

@phadej
Copy link
Collaborator

phadej commented Apr 23, 2024

One option, which is not breaking, and may fix part of #27 is to use forall a. Ord a => Ord (f a) implied by Ord1 f in the implementation, when such implication exists (i.e. more recent base).

@Bodigrim
Copy link
Author

One option, which is not breaking, and may fix part of #27 is to use forall a. Ord a => Ord (f a) implied by Ord1 f in the implementation, when such implication exists (i.e. more recent base).

Works for me. Do you want it only for Mu and Nu or for Fix also? Shall I update the PR?

@Bodigrim
Copy link
Author

Bodigrim commented Jul 4, 2024

Closing in favor of #29.

@Bodigrim Bodigrim closed this Jul 4, 2024
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.

Migrate away from Eq1/Ord1/Show1
2 participants