-
Notifications
You must be signed in to change notification settings - Fork 554
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
Scala 2.13: Fix ReverseRouting exhaustive match compilation error #25360
Conversation
The Play Framework has had reverse routing problems with match exhaustiveness before: playframework/playframework#465 Unfortunately the fix in that PR doesn't fix the compilation erros because Scala 2.13 cannot understand that this generated code does have an exhaustive match: https://gist.github.com/ioannakok/3dcf08ebffa4ebd7deb23aa1cda3f35b ``` [error] /Users/ioanna_kokkini/Projects/frontend/identity/conf/routes:23:1: match may not be exhaustive. [error] It would fail on the following input: (_, _) [error] GET /form/:formReference controllers.FormstackController.formstackForm(formReference: String, composer: Boolean = false) [error] /Users/ioanna_kokkini/Projects/frontend/applications/conf/routes:51:1: match may not be exhaustive. [error] It would fail on the following input: (_, _, _, _, _) [error] GET /embed/atom/:atomType/:id controllers.AtomPageController.render(atomType: String, id: String, isJsEnabled: Boolean = true, hasVerticalScrollbar: Boolean = false, inApp: Boolean = false) ``` One fix is to make sure that the routes are pointing to specific methods so that reverse routing doesn't try to pattern match on the parameters. We've introduced wrapper methods with different names so that the choice is explicit. We're removing the route /embed/atom/:atomType/inapp because it became obsolete in 2019: https://github.com/guardian/frontend/pull/21897/files#r941555632 We have verified that this endpoint is not called anymore: https://logs.gutools.co.uk/s/dotcom/goto/4f0cdb70-1801-11ed-b499-9b0038f50029 Co-authored-by: Roberto Tyley <roberto.tyley@guardian.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! This is extracted from the mega Scala 2.13 PR #25190, which is great because it makes that PR a bit smaller (easier to review, and less risk when it goes out!) 👍
def render( | ||
atomType: String, | ||
id: String, | ||
isJsEnabled: Boolean, | ||
hasVerticalScrollbar: Boolean, | ||
inApp: Boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seen on PROD (merged by @ioannakok 4 hours, 6 minutes and 25 seconds ago)
|
…ions/conf/routes After this PR: #25360 this commit is making the same changes in `dev-build/conf/routes` to make `dev-build` compile as this is used for local development.
The Play Framework has had reverse routing problems with match exhaustiveness before: playframework/playframework#465
Unfortunately the fix in that PR doesn't fix the compilation errors because Scala 2.13 cannot understand that this generated code does have an exhaustive match: https://gist.github.com/ioannakok/3dcf08ebffa4ebd7deb23aa1cda3f35b
One fix is to make sure that the routes are pointing to specific methods so that reverse routing doesn't try to pattern match on the parameters. We've introduced wrapper methods with different names so that the choice is explicit.
We're removing the route
/embed/atom/:atomType/inapp
because it became obsolete in 2019: https://github.com/guardian/frontend/pull/21897/files#r941555632We have verified that this endpoint is not called anymore: https://logs.gutools.co.uk/s/dotcom/goto/4f0cdb70-1801-11ed-b499-9b0038f50029