Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using #57

adampauls · 2020-03-10T01:13:37Z

Hi,

I just stumbled upon your awesome tool while building something very similar myself. I really like the way you have things set up. I wanted to add a few things and wasn't quite sure if this repo was still being actively maintained. I know it's bad practice to put multiple things in a single PR, but I wanted to get tentative approval before sending you multiple PRs. This PR adds a couple of things:

adds .apply to PathModify as an alias for .using. We use scalaftmt, which likes to break method chains up, so rather than

foo.modify(_.foo)(_ + 1)
     .modify(_.bar)(_ + 2)

we get

foo.modify(_.foo)
    .using(_ + 1)
    .modify(_.bar)
    .using(_ + 2)

adds support for .at on Option, which acts like the other at methods in that it crashes if the Option is None.
adds support for an atOrElse method on Option.
adds support for an atOrElse method on Map.

One thing I had done the with the macro I wrote was directly allow calls to Option.get, Option.getOrElse, and Map.getOrElse (and also Map.apply, but you already have that with at). I thought adding new methods was more in keeping with your approach and also required slightly less code. Curious to hear what you think!

Kind of addresses #39.

(I will also rebase before merging).

adampauls · 2020-03-10T01:13:57Z

quicklens/src/main/scala-2.13+/com.softwaremill.quicklens/package.scala

+    def atOrElse(fa: F[T], default: => T)(f: T => T): F[T]
+  }
+
+  implicit class QuicklensSingleAt[F[_], T](t: F[T])(implicit f: QuicklensSingleAtFunctor[F, T]) {


Open to a better name here.

yeah, I feel the same need for a better name, but can't come up with anything. So single it is :)

adampauls · 2020-03-10T01:14:20Z

quicklens/src/test/scala/com/softwaremill/quicklens/ModifyMapAtTest.scala

@@ -10,6 +10,12 @@ class ModifyMapAtTest extends AnyFlatSpec with Matchers {
    modify(m1)(_.at("K1").a5.name).using(duplicate) should be(m1dup)
  }

+  it should "modify a non-nested map with atOrElse" in {
+    modify(m1)(_.atOrElse("K1", A4(A5("d4"))).a5.name).using(duplicate) should be(m1dup)
+    modify(m1)(_.atOrElse("K1", ???).a5.name).using(duplicate) should be(m1dup)


Testing that we don't call the default if we don't have to.

👍 good idea

adamw · 2020-03-10T07:51:08Z

Thanks for the PR! The project is maintained and PRs are always welcome :)

I'd be wary about adding the alternative apply syntax. I understand your argument with scalafmt, however overall I think it might be confusing if there are two methods which do the same thing, when you start using the library.
what would be an example use-case of using Option.at (which can throw an exception) vs. Option.each (which traverses and does nothing if there's no element)?
atOrElse: as I understand this would cause the modified structure to be updated with a "default" value, which is further modified by subsequent calls? I'm wondering about an alternative name also, eachOrElse: maybe this would suggest that no exception is being thrown?

Looking through the features, I think we are also missing a Map.each(key) which would modify a single key, but without throwing an exception. But that's another issue for another PR :)

adampauls · 2020-03-10T16:19:04Z

Well, if I had my way, you'd deprecate using so there's only one :) Of course it's your library, so I can back that change out.
We are using quicklens to transform configurations (represented as ADTs), and it's quite surprising if a modification silently does nothing. More generally, it seems like Options, Seqs, and Maps should have both crashing and non-crashing behavior for uniformity. Right now, Options have only non-crashing behavior and Maps and Seqs only have crashing behavior for lookups.
Yes, the point of the default is to mimic the behavior of getOrElse on Maps, Options, and potentially Eithers, though I didn't bother with that yet. It allows you to write something like x.modify(_.tags.atOrElse(Set.empty))(_ ++ newTags) for tags: Option[Set[Tag]], which we want several times in our codebase.

Map.each(key) and Map.eachOrElse(key, default))/Option.eachOrElse(default) seem reasonable, although a bit confusing. I'm happy with whatever you prefer.

adamw · 2020-03-10T18:57:53Z

the project is quite old and used by a lot of people, I think the current syntax, if slightly longer, is also more readable in the long run. So I'd like to stick with it.
agreed, would be great to be uniform here, and have feature parity in both the crashing, silent and use-default options

Why would Option.eachOrElse(default) be confusing? I think we would have a nice convention of:

.each for traversing all values and doing nothing otherwise
.at for traversing values and throwing an exception if not present
.eachOrElse for traversing values with a default

Maps are another thing, we currently have Map.each which traverses all values (not giving access to keys), but traversing value at a given key makes sense as well.

adampauls · 2020-03-13T15:16:59Z

Yeah, all I meant was that eachOrElse is confusing for maps. atOrElse makes more sense there, so it might as well be atOrElse for Option too (since the crash/non-crash distinction is irrelevant because of the default). In particular, each sounds like "do this thing for each element in the collection", but the orElse is only relevant when there is nothing. .at sounds like "do something for a particular element in this collection", so even there can be only 0 or 1 elements in Option, it still makes sense to use at IMHO. But again, whatever you prefer! Let me know your final vote and I'll make the changes.

I'd like to throw in one last push for apply (as an alternative to, not replacement for, .using): I can convince my coworkers that this

this
      .modify(_.eventsReversed)(event :: _)
      .modify(_.logProb)(_ + event.score)
      .modify(_.cost)(_ + event.cost)

is better than

copy(
      event :: eventsReversed,
      logProb = logProb + event.score,
      cost = cost + event.cost,
    )

but they don't like

this
      .modify(_.eventsReversed)
      .using(event :: _)
      .modify(_.logProb)
      .using(_ + event.score)
      .modify(_.cost)
      .using(_ + event.cost)

We can use // format: off of course, but that takes away from what is otherwise perfection!

adamw · 2020-03-16T10:49:18Z

Ok, you've convinced me on at vs each :) Some of that reasoning should probably go to the docs!

We can also try with apply, although clearly marked as an alias for using, which will still be the "main" way of doing the modifications.

adampauls · 2020-03-17T16:44:27Z

README.md

@@ -56,7 +56,7 @@ import com.softwaremill.quicklens._

 case class Street(name: String)
 case class Address(street: Option[Street])
-case class Person(addresses: List[Address])
+case class Person(addresses: Seq[Address])


I was worried when I first read the docs that you had to use Lists, so I changed all the types to Seq.

Ok, let's mention Seqs as the supported collections, but keep Lists in the examples. That's the most common collection :)

adampauls · 2020-03-17T16:44:37Z

README.md

@@ -84,33 +84,55 @@ person
  .using(_.toUpperCase)
 ````

-**Modify specific sequence elements using .at:**
+**Modify specific elements in an option/sequence/map using .at:**


Merged these into one section.

adampauls · 2020-03-17T16:51:20Z

@adamw this is ready for another look. I updated the docs and added a disclaimer on apply. Let me know what you think!

adamw · 2020-03-18T20:42:47Z

Great PR - thanks! Released in 1.5.0

adampauls added 2 commits March 9, 2020 13:34

Add support for Option.get and Option.getOrElse

b2422f3

Tests.

4a026d0

adampauls commented Mar 10, 2020

View reviewed changes

adampauls changed the title ~~Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply for .using~~ Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using Mar 10, 2020

adampauls mentioned this pull request Mar 10, 2020

[Feature request, speculative] Allow arbitrary method "forwarding" #58

Closed

adampauls added 2 commits March 17, 2020 09:20

Update docs.

1329d98

Update docs.

eff4d49

adampauls commented Mar 17, 2020

View reviewed changes

adampauls added 2 commits March 17, 2020 09:46

Update docs.

17c198f

Cleanup.

827971a

adamw merged commit deb9c47 into softwaremill:master Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using #57

Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using #57

adampauls commented Mar 10, 2020 •

edited

Loading

adampauls Mar 10, 2020

adamw Mar 18, 2020

adampauls Mar 10, 2020

adamw Mar 18, 2020

adamw commented Mar 10, 2020

adampauls commented Mar 10, 2020

adamw commented Mar 10, 2020

adampauls commented Mar 13, 2020 •

edited

Loading

adamw commented Mar 16, 2020

adampauls Mar 17, 2020

adamw Mar 18, 2020

adampauls Mar 17, 2020

adampauls commented Mar 17, 2020

adamw commented Mar 18, 2020

Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using #57

Add support for .at/.atOrElse on Options, .atOrElse on Map, and .apply as an alias for .using #57

Conversation

adampauls commented Mar 10, 2020 • edited Loading

adampauls Mar 10, 2020

Choose a reason for hiding this comment

adamw Mar 18, 2020

Choose a reason for hiding this comment

adampauls Mar 10, 2020

Choose a reason for hiding this comment

adamw Mar 18, 2020

Choose a reason for hiding this comment

adamw commented Mar 10, 2020

adampauls commented Mar 10, 2020

adamw commented Mar 10, 2020

adampauls commented Mar 13, 2020 • edited Loading

adamw commented Mar 16, 2020

adampauls Mar 17, 2020

Choose a reason for hiding this comment

adamw Mar 18, 2020

Choose a reason for hiding this comment

adampauls Mar 17, 2020

Choose a reason for hiding this comment

adampauls commented Mar 17, 2020

adamw commented Mar 18, 2020

adampauls commented Mar 10, 2020 •

edited

Loading

adampauls commented Mar 13, 2020 •

edited

Loading