Skip to content
This repository has been archived by the owner on Oct 26, 2020. It is now read-only.

Change project setup to a multi-module build #14

Merged
merged 2 commits into from
Nov 19, 2019

Conversation

acceleratesage
Copy link

This is done to prepare for adding a benchmark module.
It is done as a separate PR to avoid merge problems,
as it involves a lot of renames.

  • The main build.sbt defines a root project, core sangria and shared settings
  • The sangria source has been moved to modules/core
  • The modules/core/build.sbt file defines sangria specific settings

This is done to prepare for adding a benchmark module.
It is done as a separate PR to avoid merge problems,
as it involves a lot of renames.

- The main build.sbt defines a root project, core sangria and shared settings
- The sangria source has been moved to modules/core
- The modules/core/build.sbt file defines sangria specific settings
Copy link

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I definitely think it's a good idea.

One minor point: I have a slight preference for including the core config in the core definition in the root build.sbt, rather than in core/build.sbt, for a couple of reasons:

  • It makes sharing version numbers and other config across modules more convenient.
  • It feels a little more idiomatic, or at least it seems to be the more common choice for projects with multi-module sbt builds.

@travisbrown travisbrown requested a review from yanns November 15, 2019 10:55
This will make sharing of settings and versions easier,
as noted in the code review.
@acceleratesage
Copy link
Author

@travisbrown Thanks for the review. I incorporated your suggestion and moved all config into the main build.sbt. What do you think about the factoring of commonSettings? Shall I leave it as-is or also inline them into core, as we currently do not have two projects?

@travisbrown
Copy link

@SimonAdameit That looks reasonable to me!

@paulpdaniels
Copy link

Curious if we could go further with this. I had another issue I n the old repo to split the project further into more specific modules. The reasoning being that often consumers of sangria like client projects don’t need all of the other capabilities and might only need the AST.

Copy link

@yanns yanns left a comment

Choose a reason for hiding this comment

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

looks good, thx!
I checked the created artifacts, and they seem all right.

@yanns yanns merged commit 8b55c22 into sangria-graphql-org:master Nov 19, 2019
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.

4 participants