Scala 2.13: Refactor `LiveBlogCurrentPage.findPageWithBlock()` #25338

rtyley · 2022-08-03T15:44:45Z

What does the code that's being refactored do?

LiveBlogCurrentPage.findPageWithBlock() is responsible for making sure that a user gets taken to the correct page of a liveblog when they click on a permalink to a liveblog block, eg with:

https://www.theguardian.com/politics/live/2022/aug/01/tory-leadership-race-rishi-sunak-lizz-truss-vote-keir-starmer-uk-politics-live?page=with:block-62e7e89d8f08730a1f7f5579#block-62e7e89d8f08730a1f7f5579

...the user is taken to the correct 'page' of the live blog:

page 3 of 5 for the example above:

The code was first introduced in January 2016 with #11700, subsequently updated with #13566, etc.

What's the problem?

The existing code compiles without warnings under Scala 2.12, but the Scala 2.13 compiler gives a warning that not all conceivable cases in the pattern-match are handled:

frontend/common/app/model/LiveBlogCurrentPage.scala:190:12: match may not be exhaustive.
[warn] It would fail on the following inputs: List(_), Nil
[warn]       .map {
[warn]            ^

The warning is unfortunately unwanted noise - as the preceding .sliding(3):

frontend/common/app/model/LiveBlogCurrentPage.scala

Line 188 in f90c8a5

.sliding(3)

...means that the List supplied to the case match expression will always have 3 elements, and that's precisely the case that's handled - but there's no way for the compiler to know that - so we as devs need to do something to make the warning go away!

What's the fix?

Some options:

Use Scala's @unchecked annotation. This is a last resort - turning off compiler checks leaves us exposed to runtime errors and is generally a bad idea!
Use collect() rather than map() - this accepts a partial function, rather than a total one, so the compiler error will go away.
Refactor so that the code no longer needs to assume that a List type (which could have any length) has length 3.

In the end I decided to go with the refactor, because there were a few things about the existing code that could be tweaked:

Unnecessary work: The method was creating a LiveBlogCurrentPage for every page in the liveblog, even though it only ever needed one (the single page that the block actually exists on!), and would eventually throw away the rest.

The old logic around padding the front & end of the pages List with two None entries, to allow extracting the newer & older pages, adds an extra step to the code (and therefore a bit more complexity for humans to understand: "what are endedPages?"):

frontend/common/app/model/LiveBlogCurrentPage.scala

Lines 181 to 191 in e978942

    
           val endedPages = None :: (pages.map(Some.apply) :+ None) 
        
           def hasRequestedBlock(page: LiveBlogCurrentPage): Boolean = { 
        
             page.currentPage.blocks.exists(_.id == isRequestedBlock) 
        
           } 
        
           endedPages 
        
             .sliding(3) 
        
             .toList 
        
             .map { 
        
               case List(newerPage, Some(currentPage), olderPage) =>

It can be replaced by just incrementing/decrementing the page index for the one LiveBlogCurrentPage we're creating.

Some variables (like pinnedBlock) could be inlined to the point of creation on the LiveBlogCurrentPage case class. By using named parameters in constructing the case class, clarity's retained while the code becomes more concise.

Testing

The code is covered by unit tests in LiveBlogCurrentPageTest, with additional test coverage added recently in #25118.

Here's a video showing that PROD permalinks still work on CODE when this branch is deployed:

Screen.Recording.2022-08-03.at.17.42.57.mov

See also:

The Scala 2.13 upgrade PR: Switch to Scala 2.13 #25190

@unchecked

`LiveBlogCurrentPage.findPageWithBlock()` is responsible for making sure that when a user clicks on a live-blog permalink to a **block**, eg: https://www.theguardian.com/politics/live/2022/aug/01/tory-leadership-race-rishi-sunak-lizz-truss-vote-keir-starmer-uk-politics-live?page=with:block-62e7e89d8f08730a1f7f5579#block-62e7e89d8f08730a1f7f5579 ...the user is taken to the correct 'page' of the live blog - page 3 of 5 in the example above, for instance. The code was was first introduced in January 2016 with #11700, subsequently updated with #13566, etc. The existing code compiles without warnings under Scala 2.12, but the Scala 2.13 compiler gives a warning that not _all_ conceivable cases in the pattern-match are handled: ``` frontend/common/app/model/LiveBlogCurrentPage.scala:190:12: match may not be exhaustive. [warn] It would fail on the following inputs: List(_), Nil [warn] .map { [warn] ^ ``` In this particular case, the warning's unnecessary - the preceding `.sliding(3)`: https://github.com/guardian/frontend/blob/f90c8a58e6f0941c96392d14fe27e61a4dbaf25b/common/app/model/LiveBlogCurrentPage.scala#L188 ...means that the `List` supplied to the case match expression will _always_ have 3 elements, and that's precisely the case that is handled - but there's no way for the compiler to know that - so we as devs need to do _something_ to make the warning go away! Some options: * Use the @unchecked annotation (see https://www.scala-lang.org/api/2.12.7/scala/unchecked.html) This is an option of last resort - turning off compiler checks leaves us exposed to runtime errors and is generally a bad idea! * Use `collect()` rather than `map()` - this accepts a partial function, rather than a total one, so the compiler error will go away. https://www.scala-lang.org/api/2.12.7/scala/collection/immutable/Seq.html#collectFirst[B](pf:PartialFunction[A,B]):Option[B] * Refactor so that the code no longer needs to assume that a `List` type (which can have any length) has length `3`. In the end I decided to go with the refactor, because there were a few things about the existing code that could be tweaked: * Unnecessary work: The method was creating a `LiveBlogCurrentPage` for _every_ page in the liveblog, even though it only ever needed one (the single page that the block actually exists on!), and would eventually throw away the rest. * The logic around padding the front & end of the `pages` List with two `None` entries, to allow extracting the `newer` & `older` pages, totally worked but added an extra step to the code (and therefore a bit of complexity for humans to understand: "what are endedPages?") and could be replaced by just incrementing/decrementing the page index for the one `LiveBlogCurrentPage` we're now creating. * Various variables could be inlined to the point of creation on the `LiveBlogCurrentPage` case class. By using named parameters in constructing the case class, no clarity is lost, and the code is more concise. See also: * The Scala 2.13 upgrade PR: #25190

rtyley · 2022-08-03T15:57:48Z

article/test/model/LiveBlogCurrentPageTest.scala

-      isRequestedBlock = "3",
+      requestedBlockId = "3",


The old variable name of isRequestedBlock sounds like a boolean to me, but this value is a string block id, like block-62e81de78f08c00147328d8f!

This seems like a good refactor - requestedBlockId makes a lot more sense!

rtyley · 2022-08-03T16:12:35Z

common/app/model/LiveBlogCurrentPage.scala

      filterKeyEvents: Boolean,
      topicResult: Option[TopicResult],
  ): Option[LiveBlogCurrentPage] = {
-    val pinnedBlock = blocks.find(_.attributes.pinned).map(renamePinnedBlock)


No need to calculate pinnedBlock unless we're actually going to return a LiveBlogCurrentPage instance!

rtyley · 2022-08-03T16:13:09Z

common/app/model/LiveBlogCurrentPage.scala

-    val filteredBlocks = applyFilters(blocks, filterKeyEvents, topicResult)
-    val (mainPageBlocks, restPagesBlocks) = getPages(pageSize, filteredBlocks)
-    val newestPage = FirstPage(mainPageBlocks, filterKeyEvents, topicResult)
-    val pages = newestPage :: restPagesBlocks.zipWithIndex
-      .map {
-        case (page, index) =>
-          // page number is index + 2 to account for first page and 0 based index
-          BlockPage(blocks = page, blockId = page.head.id, pageNumber = index + 2, filterKeyEvents, topicResult)


The above chunk was factored out into the new pagesFromBodyBlocks() method.

OllysCoding

+1 All of this makes sense & seems to be a good refactor - definitely was easier to understand for me than the existing code!

rtyley · 2022-08-04T10:12:38Z

Thanks!

prout-bot · 2022-08-04T10:25:44Z

Seen on PROD (merged by @rtyley 13 minutes and 13 seconds ago)

Check your changes on www.theguardian.com ✔️
Keep an eye on the deploy dashboard 📉

rtyley · 2022-08-04T10:53:50Z

Deployed and permalinks still work! 👍

Screen.Recording.2022-08-04.at.11.49.21.mov

rtyley changed the title ~~Scala 2.13: Refactor LiveBlog's findPageWithBlock()~~ Scala 2.13: Refactor LiveBlogCurrentPage.findPageWithBlock() Aug 3, 2022

rtyley mentioned this pull request Aug 3, 2022

Switch to Scala 2.13 #25190

Merged

2 tasks

rtyley marked this pull request as ready for review August 4, 2022 09:44

rtyley requested a review from a team as a code owner August 4, 2022 09:44

rtyley commented Aug 4, 2022

View reviewed changes

OllysCoding approved these changes Aug 4, 2022

View reviewed changes

rtyley merged commit 0bbbcc6 into main Aug 4, 2022

rtyley deleted the refactor-findPageWithBlock-to-simplify-and-avoid-scala-2.13-non-exhaustive-match-error branch August 4, 2022 10:12

prout-bot added Pending-on-PROD Seen-on-PROD and removed Pending-on-PROD labels Aug 4, 2022

rtyley added the Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 label Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala 2.13: Refactor `LiveBlogCurrentPage.findPageWithBlock()` #25338

Scala 2.13: Refactor `LiveBlogCurrentPage.findPageWithBlock()` #25338

rtyley commented Aug 3, 2022 •

edited

Loading

rtyley Aug 3, 2022

OllysCoding Aug 4, 2022

rtyley Aug 3, 2022

rtyley Aug 3, 2022

OllysCoding left a comment

rtyley commented Aug 4, 2022

prout-bot commented Aug 4, 2022

rtyley commented Aug 4, 2022

	val endedPages = None :: (pages.map(Some.apply) :+ None)

	def hasRequestedBlock(page: LiveBlogCurrentPage): Boolean = {
	page.currentPage.blocks.exists(_.id == isRequestedBlock)
	}

	endedPages
	.sliding(3)
	.toList
	.map {
	case List(newerPage, Some(currentPage), olderPage) =>

Scala 2.13: Refactor LiveBlogCurrentPage.findPageWithBlock() #25338

Scala 2.13: Refactor LiveBlogCurrentPage.findPageWithBlock() #25338

Conversation

rtyley commented Aug 3, 2022 • edited Loading

What does the code that's being refactored do?

What's the problem?

What's the fix?

Testing

rtyley Aug 3, 2022

Choose a reason for hiding this comment

OllysCoding Aug 4, 2022

Choose a reason for hiding this comment

rtyley Aug 3, 2022

Choose a reason for hiding this comment

rtyley Aug 3, 2022

Choose a reason for hiding this comment

OllysCoding left a comment

Choose a reason for hiding this comment

rtyley commented Aug 4, 2022

prout-bot commented Aug 4, 2022

rtyley commented Aug 4, 2022

Scala 2.13: Refactor `LiveBlogCurrentPage.findPageWithBlock()` #25338

Scala 2.13: Refactor `LiveBlogCurrentPage.findPageWithBlock()` #25338

rtyley commented Aug 3, 2022 •

edited

Loading