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

Add withDefault and atOrElse to replace None by a default value #886

Merged
merged 7 commits into from
Oct 3, 2020

Conversation

julien-truffaut
Copy link
Member

This is a slightly unlawful Iso but it is very convenient when you want to zoom into a Map like structure and set/modify the value when the key doesn't exist.

@cquiroz
Copy link
Collaborator

cquiroz commented Aug 26, 2020

Should this go into the unsafe package?

@julien-truffaut
Copy link
Member Author

Should this go into the unsafe package?

Very good question. In theory, yes, but I am tempted to propose to soften our policy with regards to "lawfullness". I mean we should definitely document when an optic or combinator is a bit dodgy and maybe use a prefix/suffix to make it clear, but if it is really useful and the criteria for "lawfullness" are most often satisfied in real-world cases, then I think we should offer it.

That's just my opinion, what do you think? and @yilinwei ?

@TimWSpence
Copy link
Contributor

TimWSpence commented Sep 30, 2020

Haskell lens has a similar but slightly different solution: https://hackage.haskell.org/package/lens-4.19.2/docs/Control-Lens-Combinators.html#v:non (if that linking doesn't work properly, the function you are looking for is
non :: Eq a => a -> Iso' (Maybe a) a

eg (super useful for nested maps)

Map.empty & at "hello" . non Map.empty . at "world" ?~ "!!!"
fromList [("hello",fromList [("world","!!!")])]

They similarly note that it's not necessarily lawful (
"Keep in mind this is only a real isomorphism if you treat the domain as being Maybe (a sans v)
This is practically quite useful when you want to have a Map where all the entries should have non-zero values."
)

The nested map situation in particular is why I'm SUPER keen on a solution to this :)

@julien-truffaut
Copy link
Member Author

@TimWSpence correct, I believe I used the name withDefault instead of non

@TimWSpence
Copy link
Contributor

TimWSpence commented Sep 30, 2020

Oh sorry @julien-truffaut, I only looked at atOrElse. Nice!! 👍

@yilinwei
Copy link
Collaborator

Sorry, didn't see this earlier.

I'm personally quite lax about coherence breakages in my own code (I've overridden an Eq instance/ Ord instances for sorted maps etc...) and I believe the argument being made is that if you consider the equivalence class where None === default, then it still follows all the laws.

We should definitely make a note of the problems in the comment - but the any unexpected breakages I suspect is where people will property test the optic rather than anything else. I'm ambivalent on the whether to have a prefix/suffix.

@julien-truffaut
Copy link
Member Author

I added tests and documentation for atOrElse let me know what you think.

core/shared/src/main/scala/monocle/function/At.scala Outdated Show resolved Hide resolved

checkAll("fromIso", AtTests[MMap[Int, String], Int, Option[String]])

checkAll("ListMap", AtTests[ListMap[Int, String], Int, Option[String]])

test("atOrElse") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to have a test for nested maps? Obviously it should just work but given that it's one of the primary motivations for this, it might be nice to test it explicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this might be better in AtExample if that is considered documentation of how to use it. Usage of at for nested maps is definitely not completely obvious at first

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. I am thinking to get rid of example module and instead use the website. It is probably easier to access.

@@ -21,6 +23,36 @@ abstract class At[S, I, A] extends Serializable {
trait AtFunctions {
def at[S, I, A](i: I)(implicit ev: At[S, I, A]): Lens[S, A] = ev.at(i)

/**
* Creates a Lens that zooms into an index `i` inside `S`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It really is a pity that there isn't a better way to link documentation in scala. These examples feel as though they belong elsewhere.

Copy link
Collaborator

@yilinwei yilinwei left a comment

Choose a reason for hiding this comment

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

LGTM

@julien-truffaut julien-truffaut merged commit 3a2ef4e into master Oct 3, 2020
@julien-truffaut julien-truffaut deleted the with-default branch March 12, 2021 08:22
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.

4 participants