Improve type-safety for the total routes.

[arturaz]

This should be source compatible but not binary compatible due to Route -> RouteImpl change.

[raquo]

Thanks! TBH initially I didn't realize that we would need to distinguish between partial and total routes to support the desired functionality, but it's obvious now.

I think your goal can be achieved in a simpler way: make Route into an abstract class, and have Route.Partial and Route.Total classes extend from it. The Route abstract class should have pretty much the entire implementation in it, with the Total and Partial subclasses containing only the methods that are specific to those use cases (probably none for Partial, and that one new method you want for Total).

I understand that this would be less flexible than using thin traits and thick impls, but I don't want to optimize for flexibility, I want to optimize for simplicity, and part of that is representing things the way we think about the domain problem. I don't think of routes as containing or deriving other routes (def backing), and I don't think of routes as something abstract that can have multiple implementations. All routes share the same implementation, total routes just have some extra methods. In this PR, it's hard to see that Route has shared implementation. It just... happens to be shared because one impl is calling into another, but the sharing is not a natural part of its structure the way a shared superclass is.

[arturaz]

Restructured as requested.

[raquo]

So, previously, withQueryAndFragment would call into withQueryAndFragmentPF, but now that's not possible because we have specialized Partial and Total classes for the two cases.

In this PR, to keep sharing the code between those implementations, you've introduced the RouteBuilder abstraction. After looking at it, I don't think it is carrying its weight. With only two usages of withQueryAndFragmentRouteBuilder, we are actually spending more lines of code (incl. the definition of withQueryAndFragmentRouteBuilder) compared to just not sharing the implementations of withQueryAndFragment and withQueryAndFragmentPF. For me, both shorter code, and fewer abstractions / indirections is better, even if it's a bit repetitive.

The same goes for other uses of RouteBuilder. IIUC, they all exist only for code sharing, but since they produce more lines of code, and more abstracted / indirect code, I think the RouteBuilder abstraction should be eliminated. Implementations should just spell out everything with no regard for code sharing, like the old def withQueryAndFragmentPF, def onlyQueryAndFragmentPF, etc. do.

[arturaz]

I originally did what you said, but then because all of the things are private inside Route, you have to have private[waypoint] def toTotal: Route.Total[Page, Args] in there and then you have to explicitly provide type arguments in the Total constructors because Scala couldn't infer them from type signature anymore, which felt just as repetive.

While the builder currently doesn't do much IMHO it's fundamentally sensible, especially if more shared code is added in the future. The other approach is just copy-pasting code, which is easy but allows for errors when modifying code. I suppose you can argue that tests should catch it?

[raquo]

I'm not sure what you mean with def toTotal – would that be a method on Partial? Why would we need it?

What I meant, for example, is that def onlyQuery could be just something like:

  new Total(
      encode = encode,
      decode = decode,
      createRelativeUrl = args => "/" + pattern.createUrlString(path = (), params = args),
      matchRelativeUrl = relativeUrl => pattern.matchRawUrl(relativeUrl).toOption.map(_.params),
      basePath = basePath
  ))

and def onlyQueryPF could have a very similar but non-shared implementation with new Partial(...). It would repeat the createRelativeUrl / matchRelativeUrl code, but those are just two usages of two lines, I'm fine with that to reduce abstraction and make the code shorter.

[raquo]

Hm. I'm not sure if it's safe to remove the == staticPage check. I have a feeling that this won't always work. For example:

trait Page { ... }
case object LoginPage extends Page

def initialPage: Page = LoginPage

val initialRoute = Route.static(initialPage, ...)

OTOH, initialRoute will capture ClassTag[Page] instead of ClassTag[LoginPage.type], and so it will match all instances of Page, not just LoginPage. This is unexpected for users, because users provide a specific initialPage for this route, so they expect the matching to happen against that page with ==, instead of by type.

I am not sure if Total is the right type for this route. Total assumes a total matching between one Page type and one args type, but here, both of those types are singletons, and the ClassTag mechanics don't seem to work well in this case.

TBH not sure what to do about this case yet. Maybe it could be a partial route instead, not sure.

[arturaz]

I've made the requested changes and introduced Route.staticTotal. While it is possible to misuse it, you have to explicitly opt-in into it and I find it useful.

[raquo]

You've replaced static with staticTotal here, but that leaves static untested. Since static is the primary method, it should retain its tests, and staticTotal tests should be added either in this file or separately.

[raquo]

So, manually specifying the AppPage in Route.staticTotal[AppPage](...) is required for the bug to manifest.

It's unfortunate, but that alone is not the entire problem. The following would also produce non-total Total routes:

case class LoginPage(spte: Option[String])

Route.staticTotal(LoginPage(step = Some("blah")), root / "login" / blah /endOfSegments)

Route.staticTotal(LoginPage(step = None), root / "login" / endOfSegments)

Scala would infer the total type to be LoginPage, but that would not be true for either of these routes, in fact there is no type that could make it a total type.

So, static pages can only be a Total route if they refer to singleton objects, AND the user did not put an incorrect type ascription. Which is the typical use case, but still, this feels rather fragile.

And yet, again, I don't see a better solution. Unless, perhaps Scala 3 macros can be used to check that the provided Page type is a singleton type? I dunno. I would be ok with that method being Scala 3 only.

As it stands with this PR, I think, most users will keep using the static method, but to add some extra warning, let's rename staticTotal to unsafeStaticTotal.

Also, can you please add a small test to show that staticTotal works as expected, and to see how exactly your a non-total version of it fails – if it's some inscrutable error, we should provide a nicer exception.

With the rename and the tests, I think we'll be able to merge it. Thanks for your work, and sorry for the delays, been very busy with another project.

[arturaz]

but to add some extra warning, let's rename staticTotal to unsafeStaticTotal.

OK, but I'll name it staticTotalUnsafe, just because Metals autocomplete doesn't search in the middle of the word, so unsafeStaticTotal wouldn't show up in autocomplete for .static, whilst .staticTotalUnsafe would.

[raquo]

Sure ok

[arturaz]

Turns out there is a way to require a type to be singleton!

https://www.scala-lang.org/api/2.13.6/scala/ValueOf.html

Thus no extra tests are needed, right?

[raquo]

TIL, that's great, we should use that for the new method. Does it mean that the user won't be able to manually provide the incorrect AppPage type ascription anymore, eliminating the safety issue? If so, I guess we can even drop unsafe from the name. Still need some very basic test, including a doesNotCompile check on the incorrect usage pattern that you mentioned in the scaladoc, to prevent regressions.

[arturaz]

Added compilation tests.

[arturaz]

Just to clarify - is there anything left to do before merging this?

[raquo]

No, everything looks good, thanks! I'll merge this and will do some other maintenance when I have time to work on OSS, probably next month.

[raquo]

@arturaz Thank you for working on this, and for the patience! I've merged this now, and I'll include this in the next version.

I looked it over again after checking it out, and noticed that after all the revisions, getClass is not needed. I replaced it entirely with ClassTag, which we were using anyway.

I'm not 100% sure what type information Java's Class encodes, but I believe the change is 100% equivalent, because:

1. in case of the singleton (staticTotal), the Page type can not contain generic params, so it's a simple straightforward case
2. in all other cases, we used to get Class from ClassTag, so they must contain the same information.

If I'm missing something, and you intended to use Class for some of its capabilities that I'm not aware of, please let me know.

[arturaz]

Looks good to me!

…

---- On Sat, 09 Nov 2024 13:16:10 +0200 Nikita Gazarov ***@***.***> wrote --- https://github.com/arturaz Thank you for working on this, and for the patience! I've merged this now, and I'll include this in the next version. I looked it over again after checking it out, and noticed that after tall the revisions, getClass is not needed. I 0218c7e#diff-72e107b3cc8eec584380b80e861f353e3c2f181372f6925272f0c5a8acfd339d it entirely with ClassTag, which we were using anyway. I'm not 100% sure what type information Java's Class encodes, but I believe the change is 100% equivalent, because: in case of the singleton (staticTotal), the Page type can not contain generic params, so it's a simple straightforward case in all other cases, we used to get Class from ClassTag, so they must contain the same information. If I'm missing something, and you intended to use Class for some of its capabilities that I'm not aware of, please let me know. — Reply to this email directly, #19 (comment), or https://github.com/notifications/unsubscribe-auth/AAADFA3E3HDS22DYOXJIPSTZ7XVHVAVCNFSM6AAAAABJTWBHMCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRWGE3TMNRYGI. You are receiving this because you were mentioned.