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

AST separation #777

Merged
merged 20 commits into from
Nov 10, 2021
Merged

AST separation #777

merged 20 commits into from
Nov 10, 2021

Conversation

performantdata
Copy link
Contributor

This involves separating the AST code into its own published module. Deprecation of what we throw away or change in the API should happen in a new 2.x branch, if desired.

This PR will help solve #461 and #466.

@performantdata performantdata force-pushed the ast-separation branch 2 times, most recently from b9465d2 to 7724189 Compare October 31, 2021 17:01
- Move `AstLocation`, `OperationType` and `QueryAst.scala` into a new
  `sangria-ast` module.

- Move the object `ast.AstNode` to `schema.AstNodeTransformer`.

- Move the `AstNode` rendering into new `QueryRenderer.renderPretty` and
  `QueryRenderer.renderCompact` methods.

- Delete the `InputDocument.to` methods. The corresponding
  `InputDocumentMaterializer.to` method should be used instead.
Remove the `analyzer`, `separateOperations`, and the `separateOperation`
methods from `Document`.  These have no place in a concrete syntax tree
representation.

Move `SourceMapper.scala` into the `ast` package.  `SourceMapper`
includes methods that return localized strings (error messages), but
those can be removed later.  This is the easiest way not to disrupt the
API too much.

Fix the tests to work with the previous commit.

Add type annotations, restrict member visibilities, documentation.
Clean up IDE and build warnings.
@performantdata performantdata marked this pull request as ready for review November 3, 2021 06:54
@yanns
Copy link
Contributor

yanns commented Nov 3, 2021

I'll try to use a snapshot later to check how important the breaking changes are.

build.sbt Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

I've opened a PR with some suggestions: performantdata#3

build.sbt Outdated Show resolved Hide resolved
@performantdata
Copy link
Contributor Author

performantdata#3 is cherry-picked.

Copy link
Contributor

@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.

This is looking better & better! 👍

@performantdata
Copy link
Contributor Author

:shipit:

@performantdata
Copy link
Contributor Author

How/when does the CHANGELOG get updated?

@performantdata
Copy link
Contributor Author

This should be added to the milestone. @yanns

Copy link
Contributor

@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.

Great work!
Can you please remove the references to other libraries? Thanks!

modules/core/src/main/scala/sangria/package.scala Outdated Show resolved Hide resolved
@yanns yanns added this to the 3.0.0 milestone Nov 10, 2021
@yanns
Copy link
Contributor

yanns commented Nov 10, 2021

How/when does the CHANGELOG get updated?

manually, when we can.

Copy link
Contributor

@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.

💯

@yanns yanns merged commit 5a342c2 into sangria-graphql:main Nov 10, 2021
@performantdata performantdata deleted the ast-separation branch November 10, 2021 16:12
@yanns
Copy link
Contributor

yanns commented Nov 14, 2021

This change breaks some projects: #795

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

Successfully merging this pull request may close these issues.

2 participants