[Navigation doc] differenciate route vs route pattern #508
Conversation
| * @param route The topmost destination to retain. May contain filled in arguments as long as | ||
| * it is exact match with route used to navigate. | ||
| * @param route The route of the topmost destination to retain. Both route patterns | ||
| * and route instances with filled in arguments are supported. |
There was a problem hiding this comment.
The use of "route pattern" and "route" here should be distinctive enough. Can you also clarify that if using route, it must match the original route that was used to navigate?
There was a problem hiding this comment.
Agree on "route pattern" and "route" 👍
I removed the May contain filled in arguments as long as it is exact match with route used to navigate. because I felt like this was the general expectation of the method and didn't add much information. Whether using a route pattern or a route, the expectation is that it returns true if found or false if not. Or did I get this wrong?
There was a problem hiding this comment.
returns true if found or false if not is indeed the case. In the past we matched based on route pattern and didn't differentiate between "route/myArg1" and "route/myArg2". Now that matching takes into account filled in arguments, matching behavior has changed and what was previously expected to match may no longer match. Therefore the clarification could be important. Also, it doesn't hurt to have more docs than less :).
There was a problem hiding this comment.
Kind of disagree. KDoc is a place for reference information and changes belong in the changelog. Also more docs can hurts. In my personal case, this sentence got me puzzled about whether this would throw if passed the wrong route or how come there was an extra sentence in addition to the return value.
I don't want to die on this above hill though! If you think it is important to keep the sentence, please confirm and I'l re-add it (or feel free to edit directly before merging too!)
| @@ -581,7 +581,8 @@ public open class NavController( | |||
| * Attempts to pop the controller's back stack back to a specific destination. This does | |||
| * **not** handle calling [dispatchOnDestinationChanged] | |||
| * | |||
| * @param route The topmost destination with this route to retain | |||
| * @param route The route of the topmost destination to retain. Both route patterns | |||
| * and route instances with filled in arguments are supported. | |||
There was a problem hiding this comment.
Same as above, lets stick to "route pattern" and "route".
| * `saveState` value of `true`. May contain filled in arguments as long as | ||
| * it is exact match with route used with [popBackStack]. | ||
| * `saveState` value of `true`. Both route patterns and route instances with filled in | ||
| * arguments are supported. |
There was a problem hiding this comment.
Same comment as with popBackStack and likewise for getBackStackEntry.
| @@ -2143,7 +2144,7 @@ public open class NavController( | |||
| * Navigate to a route in the current NavGraph. If an invalid route is given, an | |||
| * [IllegalArgumentException] will be thrown. | |||
| * | |||
| * @param route route for the destination | |||
| * @param route route instance with filled arguments for the desired destination | |||
There was a problem hiding this comment.
It accepts both route with filled arguments or the route patterns.
Given a destination with route "route/{arg}" you can navigate with either "route/{arg}" or "route/myArg".
Suggest: "route with filled arguments or the route pattern for the destination".
There was a problem hiding this comment.
Oh nice, TIL! I tweaked your suggestion a bit to put "route pattern or route" (always put route pattern first for consistency)
| @@ -2157,7 +2158,7 @@ public open class NavController( | |||
| * Navigate to a route in the current NavGraph. If an invalid route is given, an | |||
| * [IllegalArgumentException] will be thrown. | |||
| * | |||
| * @param route route for the destination | |||
| * @param route route instance with filled arguments for the desired destination | |||
|
You can safely ignore the lint failure here - it's on our end. |
|
Changes pushed. I'm not 100% sure about |
This is a docs-only change
This PR tries to highlight the difference between :
"topic/{topicId}""topic/42"Since changing argument names would be a breaking change, this PR tweaks the KDoc.
Test: N/A docs change