Switch Left and Right in Eithers #25950
Conversation
bryophyta
force-pushed
the
pf/left-to-right
branch
from
fc91d58
to
73c59de
Compare
bryophyta
changed the title
bryophyta
force-pushed
the
pf/left-to-right
branch
from
73c59de
to
32a7584
Compare
|
I think your link to the Principle of least astonishment is not quite right: https://github.com/guardian/recommendations/blob/main/coding-with-empathy.md#recommendations This made me chuckle: scala/scala#6682 (comment) |
Good call, thanks! (Interestingly, it looks like I forgot to put a URL in at all, and so |
There was a problem hiding this comment.
馃憤 this is a great refactor @bryophyta, and I think it will definitely help make this code more readable
|
Thanks everyone for the reviews! |
|
Overdue on FRONTS-PROD, ADMIN-PROD (merged by @bryophyta 30 minutes and 9 seconds ago) What's gone wrong? |
|
Seen on FRONTS-PROD (merged by @bryophyta 40 minutes and 32 seconds ago)
|
|
Seen on ADMIN-PROD (merged by @bryophyta 40 minutes and 58 seconds ago)
|
What does this change?
Switches the
LeftandRighttypes associated with theModelOrResultobject, and refactors the controllers which use this object (either directly or indirectly).Why?
The convention in Scala is to use
Rightas the 'success' condition, as noted in this comment on theModelOrResultobject:https://github.com/guardian/frontend/blame/f5604316a946a4113807de3fe2af74ed79307877/common/app/common/ModelOrResult.scala#L10-L12
As of Scala 2.13 it looks like treating
Rightas the error case is deprecated and elsewhere in Frontend we useRightas the success case:https://github.com/guardian/frontend/blob/0657f594e2fd51125d28df9048c48d6eede26bb6/admin/app/model/deploys/RiffRaff.scala#L49-50
So in accordance with our recommendation to follow the principle of least surprise in our code, it makes sense to standardise on the broader convention, where possible. (As much as it pains me, as a left-handed developer 馃槩馃槄.)
Testing
I've done a test deployment in CODE and clicked around a bit, including in apps like Preview which sometimes have surprising dependencies on e.g. Article code. There were no obvious issues, and I'm hoping that this is a case where the type system and tests will give us some security. But having said that, it feels like quite a big (and not-strictly-necessary) change, so I'd appreciate any thoughts about whether we can be confident that won't break anything!