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

added Unlift derivation for reader-like effects with subcontexts #367

Merged
merged 5 commits into from
Sep 18, 2020
Merged

added Unlift derivation for reader-like effects with subcontexts #367

merged 5 commits into from
Sep 18, 2020

Conversation

klimovsv
Copy link
Contributor

@klimovsv klimovsv commented Sep 3, 2020

No description provided.

danslapman
danslapman previously approved these changes Sep 3, 2020
View reviewed changes
Copy link
Collaborator

@danslapman danslapman left a comment

Choose a reason for hiding this comment

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

LGTM

oskin1
oskin1 previously approved these changes Sep 4, 2020
View reviewed changes
@catostrophe
Copy link
Member

@Odomontois you said "Что-то боюсь его имплиситом делать"

@Odomontois
Copy link
Member

Odomontois commented Sep 9, 2020 •
edited
Loading

Yeah. This function looks really good, but the amount of type parameters, that need to be inferred through the implicit search makes me think it would be rarely found.
Moreover, it could create conflicts with zioTofuContainsUnliftImplicit if it is reexported through context companion. That claim should be investigated though.
So while I've found this very helpful, I'd like to make this def subContextUnlift non-implicit at least for a while.
@klimovsv what do you think?

@klimovsv
Copy link
Contributor Author

klimovsv commented Sep 10, 2020 •
edited
Loading

@Odomontois I am okay with that. I think we should compare this derived instance with same instances for Env and ZIO. May be we could remove particular instances in favor of this derivation.

@Odomontois
Copy link
Member

@klimovsv could you please make this def non-implicit, so I could merge now

@klimovsv klimovsv dismissed stale reviews from oskin1 and danslapman via a3ccc02 September 17, 2020 20:23
@Odomontois Odomontois merged commit 92471a3 into tofu-tf:master Sep 18, 2020
@Odomontois Odomontois added the enhancement New feature or request label Sep 23, 2020
@catostrophe catostrophe mentioned this pull request Sep 24, 2020
Merged
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

@danslapman danslapman danslapman left review comments

@oskin1 oskin1 oskin1 left review comments

@Odomontois Odomontois Awaiting requested review from Odomontois Odomontois 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.
5 participants
@klimovsv @catostrophe @Odomontois @danslapman @oskin1