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.

Sign up for GitHub

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

Jump to bottom

Auto-summonable optics instances for Errors #276

Merged
merged 9 commits into from
May 28, 2020
Merged

Auto-summonable optics instances for Errors #276

merged 9 commits into from
May 28, 2020

Conversation

oskin1
Copy link
Collaborator

@oskin1 oskin1 commented May 24, 2020

No description provided.

@oskin1 oskin1 changed the title Upcast instance for Raise autosummon. Auto-summonable Upcast instance for `Raise. May 24, 2020
@oskin1 oskin1 changed the title Auto-summonable Upcast instance for `Raise. Auto-summonable Upcast instance for Raise May 24, 2020
catostrophe
catostrophe reviewed May 24, 2020
View reviewed changes
Copy link
Member

@catostrophe catostrophe left a comment

Choose a reason for hiding this comment

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

Could you also make Subset[E, E1] auto-summonable?

@oskin1
Copy link
Collaborator Author

oskin1 commented May 24, 2020 •
edited
Loading

Could you also make Subset[E, E1] auto-summonable?

@catostrophe , what are the use cases where Upcast[E, E1] instance is not enough and Subset[E, E1] is required?

It seems that whenever we can auto-derive Sebset[E, E1] for E1 <: E we can also derive Upcast[E, E1], so why do we need Sebset auto-summoning?

@catostrophe
Copy link
Member

Could you also make Subset[E, E1] auto-summonable?

@catostrophe , what are the use cases where Upcast[E, E1] instance is not enough and Subset[E, E1] is required?

It seems that whenever we can auto-derive Sebset[E, E1] for E1 <: E we can also derive Upcast[E, E1], so why do we need Sebset auto-summoning?

Subset is just a prism. It is used in error handling. Why don't we have an autosummonable Subset instance for ADTs yet?

@catostrophe
Copy link
Member

Subset is just a prism. It is used in error handling. Why don't we have an autosummonable Subset instance for ADTs yet?

And we cannot share Subset and Upcast instances because the former requires ClassTag for patmatching on error subtype.

Odomontois
Odomontois previously approved these changes May 25, 2020
View reviewed changes
@oskin1 oskin1 changed the title Auto-summonable Upcast instance for Raise Auto-summonable optics instances for Errors May 25, 2020
catostrophe
catostrophe reviewed May 25, 2020
View reviewed changes
core/src/test/scala/tofu/examples/TofuRaiseExamples.scala
@@ -0,0 +1,84 @@
package tofu.examples
Copy link
Member

Choose a reason for hiding this comment

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

I suggest narrowing package namespace. Otherwise classes from different examples may clash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@catostrophe, can you be more specific?

Copy link
Member

Choose a reason for hiding this comment

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

@oskin1 Sorry. This doesn't seem to be an issue. I meant classes in the tofu.examples package in different modules might clash.

@Odomontois Odomontois merged commit a5a5553 into tofu-tf:master May 28, 2020
@catostrophe catostrophe added the enhancement New feature or request label May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catostrophe catostrophe catostrophe approved these changes

@Odomontois Odomontois Odomontois left review comments

@danslapman danslapman Awaiting requested review from danslapman danslapman is a code owner

@Z1kkurat Z1kkurat Awaiting requested review from Z1kkurat

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oskin1 @catostrophe @Odomontois