Skip to content
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

server: add hlint hints to replace case analysis with combinators #6145

Merged
merged 3 commits into from
Nov 9, 2020

Conversation

abooij
Copy link
Contributor

@abooij abooij commented Nov 6, 2020

Description

This PR implements 8 new hlint hints that enforce usage of our onNothing and onLeft combinators. Thus it further improves on #6126. Example hints provided by these new hlint hints:

src-lib/Hasura/RQL/Types/Common.hs:(292,20)-(294,57): Warning: Use onNothing
Found:
  case x of
    Just v -> return v
    Nothing -> fail "integer passed is out of bounds"
Perhaps:
  onNothing x (fail "integer passed is out of bounds")
src-lib/Hasura/RQL/DDL/RemoteRelationship/Validate.hs:(434,7)-(436,25): Warning: Use onLeft
Found:
  case eitherScalar of
    Left _ -> throwError $ InvalidGraphQLName $ toSQLTxt scalarType
    Right s -> pure s
Perhaps:
  onLeft
    eitherScalar
    (\ _c -> throwError $ InvalidGraphQLName $ toSQLTxt scalarType)

These new hints are triggered 23 times on the current master codebase.

This PR also removes some dead code, namely coerceSet, from Hasura.Prelude.

Affected components

  • Server
  • Tests

Comment on lines -145 to -154
-- | Efficiently coerce a set from one type to another.
--
-- This has the same safety properties as 'Set.mapMonotonic', and is equivalent
-- to @Set.mapMonotonic coerce@ but is more efficient. This is safe to use when
-- both @a@ and @b@ have automatically derived @Ord@ instances.
--
-- https://stackoverflow.com/q/57963881/176841
coerceSet :: Coercible a b=> Set.Set a -> Set.Set b
coerceSet = unsafeCoerce

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this is removed from here. Is this change relevant to this PR?

Copy link
Contributor Author

@abooij abooij Nov 6, 2020

Choose a reason for hiding this comment

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

No, it's not relevant. Would you rather see this change be merged separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's just dead code removal. It's fine this way

Copy link
Contributor

Choose a reason for hiding this comment

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

We should check that Pro doesn't use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicuveo According to some quick greps, it's not used by pro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, then. LGTM! :)

Copy link
Contributor

@codingkarthik codingkarthik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tirumaraiselvan tirumaraiselvan left a comment

Choose a reason for hiding this comment

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

no cl required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants