graphql-api icon indicating copy to clipboard operation
graphql-api copied to clipboard

Simplify code by using DuplicateRecordFields and NamedFieldPuns

Open zhujinxuan opened this issue 6 years ago • 3 comments

Recently I am working on https://github.com/haskell-graphql/graphql-api/pull/205, and I find that I have to manually add _ everywhere for adding position infomation. And we can see quite a lot repetitive code like (in Validation.hs)

-- | Get the selection set for an operation.
getSelectionSet :: Operation value -> SelectionSetByType value
getSelectionSet (Query _ _ ss) = ss
getSelectionSet (Mutation _ _ ss) = ss

However, we can simplify them with DuplicateRecordFields and NamedFieldPuns. For example, we can have the following code in Validation.hs that:

{-# DuplicateRecordFields #-}

data Operation value
  = Query {
       _ss :: VariableDefinitions
      , _dv :: (Directives value) 
      , getSelectionSet :: (SelectionSetByType value)
  | Mutation {
      _ss :: VariableDefinitions 
      , _dv :: (Directives value)
      , getSelectionSet :: (SelectionSetByType value)
   } deriving (Eq, Show)

Furthermore, with {-# NamedFieldPuns #-}, we can simplify some other code from

    splitOps (AST.Query node@(AST.Node maybeName _ _ _ _) _) = Right (maybeName, (Query, node))

to

{-# NamedFieldPuns #-}
    splitOps AST.Query {_node = [email protected] {_name = maybeName}}  = Right (maybeName, (Query, node))

Personally, I think by that we can make code easier to write without carefully counting the number of _s, and take advantage with other language extensions of Records when developing with graphql.

zhujinxuan avatar Nov 02 '18 18:11 zhujinxuan

Very strongly in favour of this. If I remember rightly, when we wrote this, @teh and I disagreed about whether we should prefer pattern matching (as described above) or field accessor functions. We only latched on to NamedFieldPuns later on in the piece.

jml avatar Nov 06 '18 10:11 jml

I can't even remember that discussion but it does seem like a sensible change!

teh avatar Nov 07 '18 20:11 teh

@teh , @jml Great. I will do that change after #205 is finished.

zhujinxuan avatar Nov 07 '18 22:11 zhujinxuan