Skip to content
This repository was archived by the owner on Apr 4, 2024. It is now read-only.

Simplify pattern match for patch_preflight #1671

Conversation

Gabriella439
Copy link
Contributor

This one is easier to explain if you format the pattern match a bit:

case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { true        , true           , false            , _              , :sufficient  , true                , _                       } -> :waiting
     { true        , true           , _                , false          , :sufficient  , true                , _                       } -> :waiting
     { _           , _              , _                , _              , :sufficient  , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end

Notice how after the second case of the pattern match the first value must necessarily be true, so you can convert those to wildcard pattern matches:

case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { _           , true           , false            , _              , :sufficient  , true                , _                       } -> :waiting
     { _           , true           , _                , false          , :sufficient  , true                , _                       } -> :waiting
     { _           , _              , _                , _              , :sufficient  , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end
     # ↑ s/true/_/ after the first false

If you keep repeating this process (of replacing pattern matches that are guaranteed to succeed with wildcards) then you get this sequence of pattern matches:

case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { _           , _              , false            , _              , _            , _                   , _                       } -> :waiting
     { _           , _              , _                , false          , _            , _                   , _                       } -> :waiting
     { _           , _              , _                , _              , _            , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end

… which is clearer (IMO) because now it basically reads as:

  • either all conditions pass or …
  • one of the conditions individually fails

… which makes it more clear that this sequence of pattern matches is exhaustive.

This one is easier to explain if you format the pattern match a bit:

```elixir
case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { true        , true           , false            , _              , :sufficient  , true                , _                       } -> :waiting
     { true        , true           , _                , false          , :sufficient  , true                , _                       } -> :waiting
     { _           , _              , _                , _              , :sufficient  , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end
```

Notice how after the second case of the pattern match the first value
must necessarily be `true`, so you can convert those to wildcard
pattern matches:

```elixir
case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { _           , true           , false            , _              , :sufficient  , true                , _                       } -> :waiting
     { _           , true           , _                , false          , :sufficient  , true                , _                       } -> :waiting
     { _           , _              , _                , _              , :sufficient  , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end
     # ↑ s/true/_/ after the first false
```

If you keep repeating this process (of replacing pattern matches that
are guaranteed to succeed with wildcards) then you get this sequence of
pattern matches:

```elixir
case { passed_label, no_error_status, no_waiting_status, no_unset_status, passed_review, code_owners_approved, passed_up_to_date_review} do
     { true        , true           , true             , true           , :sufficient  , true                , :sufficient             } -> {:ok, toml.max_batch_size}
     { false       , _              , _                , _              , _            , _                   , _                       } -> {:error, :blocked_labels}
     { _           , _              , _                , _              , :insufficient, _                   , _                       } -> {:error, :insufficient_approvals}
     { _           , _              , _                , _              , :failed      , _                   , _                       } -> {:error, :blocked_review}
     { _           , _              , _                , _              , _            , false               , _                       } -> {:error, :missing_code_owner_approval}
     { _           , false          , _                , _              , _            , _                   , _                       } -> {:error, :pr_status}
     { _           , _              , false            , _              , _            , _                   , _                       } -> :waiting
     { _           , _              , _                , false          , _            , _                   , _                       } -> :waiting
     { _           , _              , _                , _              , _            , _                   , :insufficient           } -> {:error, :insufficient_up_to_date_approvals}
end
```

… which is clearer (IMO) because now it basically reads as:

- either all conditions pass or …
- one of the conditions individually fails

… which makes it more clear that this sequence of pattern matches is
exhaustive.
@notriddle notriddle enabled auto-merge June 1, 2023 22:47
@notriddle notriddle added this pull request to the merge queue Jun 1, 2023
Merged via the queue into bors-ng:master with commit fdf3d27 Jun 1, 2023
notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants