💥 matchRawUrl of PathSegment now demands that full path is matched #25
Conversation
sherpal
force-pushed
the
breaking-matchRawUrl-change
branch
from
891596e to
5847668
Compare
There was a problem hiding this comment.
Nice, This seems fine! Just some small comments.
You mentioned:
unfortunately I will need to embed the endOfSegmentRequired within the pathsegment...
Just FTR, I'm gonna confess that the reason why it's unfortunate escapes me at the moment – perhaps I haven't looked deeply / intently enough. You definitely know this project better though, especially the error handling that I've never really used, so if that part looks ok to you, that's good enough for me 👍
| val remainingSegments: PathSegment[List[String], A] = PathSegment.remainingSegments | ||
| lazy val root: PathSegment[Unit, A] = PathSegment.root | ||
| lazy val remainingSegments: PathSegment[List[String], A] = PathSegment.remainingSegments | ||
| lazy val ignoreRemaining: PathSegment[Unit, A] = remainingSegments.ignore(Nil) | ||
| lazy val endOfSegments: PathSegment[Unit, A] = PathSegment.endOfSegments |
There was a problem hiding this comment.
Should probably @deprecate endOfSegments since there is no more use for it... or is there?
There was a problem hiding this comment.
Ah yes I wanted to do that, thanks
| val remainingSegments: PathSegment[List[String], A] = PathSegment.remainingSegments | ||
| lazy val root: PathSegment[Unit, A] = PathSegment.root | ||
| lazy val remainingSegments: PathSegment[List[String], A] = PathSegment.remainingSegments | ||
| lazy val ignoreRemaining: PathSegment[Unit, A] = remainingSegments.ignore(Nil) |
There was a problem hiding this comment.
Naming comment: hmm ignoreRemaining seems unclear on its own – remaining what? Especially when used as a property, without the /. Maybe ignoreRemainingSegments? – even if it's a mouthful, it would be clearer and consistent. Usages of it should be pretty rare anyway...
Eh TBH not super sure about this. I can see either way being fine. It just seems that ignoreRemaining was chosen to be consistent with the def ignore name, but as I mentioned in another comment, we probably shouldn't have def ignoreRemaining in the first place. So if that method does go away, perhaps naming should be realigned to be consistent with remainingSegments.
There was a problem hiding this comment.
I chose ignoreRemaining over ignoreRemainingSegments because I thought it was too long, but that's indeed not a good point. It's better to be more explicit.
| @@ -169,19 +193,21 @@ trait PathSegment[T, +A] extends UrlPart[T, A] { | |||
| (_: Unit) => createSegments(default) | |||
| ) | |||
|
|
|||
| final def ignoreRemaining: PathSegment[T, A] = this / PathSegment.remainingSegments.ignore(Nil) | |||
There was a problem hiding this comment.
This method seems a bit out of place / redundant, no? The other non-symbolic methods here (like ignore) are about modifying the handling of the current segment or path, whereas def ignoreRemaining just appends another segment – but we already have / syntax for that.
| pathSegment: PathSegment[PathType, PathError], | ||
| queryParams: QueryParameters[ParamsType, ParamsError] |
There was a problem hiding this comment.
If we're making breaking changes, could you please expose these constructor params as val? Or is there a good reason not to? I feel like this will eventually come in handy for route transformations in Waypoint...
Previously, the `matchRawUrl` method of `PathSegment` would succeed if there were unused segments in the path. We change that behaviour in such a way that `matchRawUrl` now **fails** if there are unused segments after applying the path. Users who want to go back to the old behaviour should use the new `ignoreRemaining` method. See [issue](#24)
sherpal
force-pushed
the
breaking-matchRawUrl-change
branch
from
5847668 to
78721e8
Compare
Previously, the
matchRawUrlmethod ofPathSegmentwould succeed if there were unused segments in the path.We change that behaviour in such a way that
matchRawUrlnow fails if there are unused segments after applying the path. Users who want to go back to the old behaviour should use the newignoreRemainingmethod.See issue
Main attention points: