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

Relax type constraint for QuicklensMapAt #78

Merged
merged 2 commits into from
Dec 22, 2021
Merged

Conversation

LMnet
Copy link

@LMnet LMnet commented Dec 3, 2021

I relaxed a type constraint for implicit class QuicklensMapAt. Previously it works only on subtypes of the Map type. With this PR it will work for any type with the instance of QuicklensMapAtFunctor typeclass.

Real-world use-case: newtype wrapping Map. It may not have subtype relationships but could work like a Map. In my codebase, I have a custom NonEmptyMap, for example.

Relaxing type constraint leads to an implicit resolution ambiguity: compiler can't choose between QuicklensFunctor and QuicklensMapAtFunctor for each calls. To fix this I moved implicit class QuicklensEach into the LowPriorityImplicits trait.

Overall it is a safe change in terms of API. All old code will work without changes.

@adamw
Copy link
Member

adamw commented Dec 6, 2021

Thank you! Could you maybe add a test, which would somehow verify that the new functionality works?

I'd also add a comment as to why the .each in a low-priority implicit, as it might not be obvious looking at the code.

@LMnet
Copy link
Author

LMnet commented Dec 10, 2021

@adamw done.

During the test implementation, I noticed that in the scala 3 code QuicklensMapAt is renamed to QuicklensIndexedFunctor and it is not affected by the original problem. So I have to create a separate scala-2 test folder to test my addition.

@adamw adamw merged commit 772266c into softwaremill:master Dec 22, 2021
@adamw
Copy link
Member

adamw commented Dec 22, 2021

Should be released soon in 1.8.0, thanks! :)

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.

2 participants