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

refactor schema.hs into multiple modules #2661

Merged
merged 3 commits into from
Aug 9, 2019

Conversation

0x777
Copy link
Member

@0x777 0x777 commented Aug 2, 2019

Schema.hs has become a little unwieldy over the past few months. This PR does not attempt to fix all the issues with Schema.hs has but sets up a base for further improvements. This PR does two things:

  1. Functions/types present in incorrect modules (which I believe was done to avoid cyclic dependencies) are now moved to appropriate ones.
  2. Schema.hs is split into multiple sub-modules.

@netlify
Copy link

netlify bot commented Aug 2, 2019

Deploy preview for hasura-docs ready!

Built with commit 08282ad

https://deploy-preview-2661--hasura-docs.netlify.com

@hasura-bot
Copy link
Contributor

Review app for commit 31ded71 deployed to Heroku: https://hge-ci-pull-2661.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2661-31ded710

@@ -215,6 +215,16 @@ library
, Hasura.GraphQL.Transport.WebSocket.Protocol
, Hasura.GraphQL.Transport.WebSocket.Server
, Hasura.GraphQL.Transport.WebSocket
, Hasura.GraphQL.Schema.BoolExp
Copy link
Member Author

@0x777 0x777 Aug 5, 2019

Choose a reason for hiding this comment

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

The code from Schema.hs is more or less moved into these new modules without any functionality change.


type OpCtxMap = Map.HashMap G.Name OpCtx

data InsOpCtx
Copy link
Member Author

Choose a reason for hiding this comment

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

These types are related to the Resolve module and they have been moved to Types.hs in Resolve.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the same time ContextTypes.hs has been renamed/merged into Types.hs with the above inclusions.


emptyGCtx :: GCtx
emptyGCtx = mkGCtx mempty mempty mempty
emptyGCtx =
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously creating an empty GCtx was done with mkGCtx which lead to all sorts of cyclic dependencies and code being in incorrect modules. Now emptyGCtx is implemented separately with both mkGCtx and emptyGCtx sharing the common functionality (this could be improved further though).

, UpdPermForIns(..)
, InsCtx(..)
, InsCtxMap
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these are now part of Resolve.Types module and that is exposed by this module.

} deriving (Show, Eq)
$(J.deriveJSON (J.aesonDrop 3 J.snakeCase) ''InsResp)

data AnnPGVal
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to Types

@@ -0,0 +1,54 @@
module Hasura.GraphQL.Schema.Common
Copy link
Member Author

Choose a reason for hiding this comment

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

Common for lack of a better name. These functions/values are imported by more than one sub-module under Schema.

@@ -0,0 +1,52 @@
module Hasura.GraphQL.Schema.Mutation.Common
Copy link
Member Author

@0x777 0x777 Aug 5, 2019

Choose a reason for hiding this comment

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

Further hierarchy for Mutation

@0x777 0x777 marked this pull request as ready for review August 5, 2019 10:37
@0x777 0x777 requested a review from lexi-lambda as a code owner August 5, 2019 10:37
rakeshkky
rakeshkky previously approved these changes Aug 8, 2019
Copy link
Member

@rakeshkky rakeshkky left a comment

Choose a reason for hiding this comment

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

changes LGTM

lexi-lambda
lexi-lambda previously approved these changes Aug 8, 2019
Copy link
Contributor

@lexi-lambda lexi-lambda left a comment

Choose a reason for hiding this comment

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

This looks great to me; thank you for taking the time to do this!

@hasura-bot
Copy link
Contributor

Review app for commit 08282ad deployed to Heroku: https://hge-ci-pull-2661.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2661-08282ad1

@0x777 0x777 merged commit 52bf885 into hasura:master Aug 9, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-2661.herokuapp.com is deleted

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
Mostly moving around things across modules. No change in
functionality.
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.

5 participants