Skip to content

Conversation

supersven
Copy link
Contributor

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If HTTP endpoint paths have been added or renamed, the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.
    • If new config options introduced: added usage description under docs/reference/config-options.md
    • If new config options introduced: recommended measures to be taken by on-premise instance operators.
    • If a cassandra schema migration is backwards incompatible (see also these docs), measures to be taken by on-premise instance operators are explained.
    • If a data migration (not schema migration) introduced: measures to be taken by on-premise instance operators.
    • If public end-points have been changed or added: does nginz need un upgrade?
    • If internal end-points have been added or changed: which services have to be deployed in a specific order?

throw InvalidRoute
-- check that the path has the expected form
(componentSeg, rpcPath) <- case Wai.pathInfo req of
["federation", comp, rpc] -> pure (comp, rpc)
_ -> throw InvalidRoute

when (not (Text.all isAllowedRPCChar rpcPath)) $
unless (Text.all isAllowedRPCChar rpcPath) $
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, but I doubt that it's non-controversial. :) Let's see if anybody complains.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem to be exactly the opposite of each other:

when p s = if p then s else pure ()

unless p s =  if p then pure () else s

So, I'm wondering how this could be controversial. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's clearly still correct, just a question of style: is code easier to read if with fewer keywords, or with less complexity in boolean expressions? i'm also leaning towards unless, and i think we're way too deep into a bike shedding thing again here, so just ignore me. if somebody feels strongly, they can make a PR with an hlint rule that rolls this back, then we can bike shed. :)

@fisx
Copy link
Contributor

fisx commented Dec 24, 2021

(please add a changelog line.)

@supersven supersven force-pushed the sventennie/hlint-federator branch from 9b76041 to 6c3fbf9 Compare December 27, 2021 08:46
@fisx
Copy link
Contributor

fisx commented Dec 28, 2021

good to merge!

@supersven supersven merged commit 0798bd3 into develop Dec 28, 2021
@supersven supersven deleted the sventennie/hlint-federator branch December 28, 2021 12:10
@akshaymankar akshaymankar mentioned this pull request Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants