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

Issue #4631: Make Later covariant #4632

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Conversation

yanns
Copy link
Contributor

@yanns yanns commented Jul 4, 2024

Same as #937

fixes #4631

@satorg
Copy link
Contributor

satorg commented Jul 4, 2024

Thank you for the fix!
Note that Later is one of the Leaf descendants. The other two are Always and Now.
I wonder if it makes sense to make all the "leaves" covariant together rather than just one of them?
I feel it could be rather confusing to have one Leaf covariant and the other two invariant.

@yanns
Copy link
Contributor Author

yanns commented Jul 5, 2024

Thank you for the fix! Note that Later is one of the Leaf descendants. The other two are Always and Now. I wonder if it makes sense to make all the "leaves" covariant together rather than just one of them? I feel it could be rather confusing to have one Leaf covariant and the other two invariant.

I had the same thoughts when changing Later, but was not feeling confident enough to make more changes than the very strict necessary. Thanks for the quick feedback! I've now adapted all leafs of Eval.

@satorg
Copy link
Contributor

satorg commented Sep 1, 2024

@yanns , I lost tracking of your PR, sorry. Would you mind updating it to the most recent "main" such that we could re-run all the checks please? Thank you!

@yanns
Copy link
Contributor Author

yanns commented Sep 2, 2024

@satorg thanks for coming back! I've updated the branch. The tests on scala 2.12 with scala native are failing, without any good error message.

@yanns
Copy link
Contributor Author

yanns commented Sep 2, 2024

now all tests have passed

Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

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

Thank you!

@yanns
Copy link
Contributor Author

yanns commented Sep 3, 2024

When does a PR get merged? I could find any process described in https://github.com/typelevel/cats/blob/main/CONTRIBUTING.md ?
Do you wait for 2 reviews?

@satorg satorg merged commit 927a9bb into typelevel:main Sep 4, 2024
16 checks passed
@yanns yanns deleted the 4631_covariant_later branch September 4, 2024 08:05
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.

Make Later covariant in its type parameter
3 participants