Skip to content

Conversation

@chriswr95
Copy link
Contributor

@chriswr95 chriswr95 commented Oct 23, 2021

Hello from Twitter!

About

I'm working on a task for my overlord @jbellenger to prune unused fragments from a Document. To accomplish this, I'm hoping to pass a context argument into the AstTransfromer which will provide the names of the nodes to prune. This seems like functionality other users may want from AstTransfromer so I'm raising a pull request to add it.

What Changed

I overloaded the .transform() method of AstTransformer so that it accepts a "rootVars" object, and passes it to the TreeTransformer.

This is my first time contributing to graphql-java, or any open source project, so please let me know if I should add other details before raising a PR in the future. Looking forwards to working with y'all.

Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

I would love it if you could clean up the JavaDoc to improve it please

return TraversalControl.CONTINUE;
}
};
public Node transform(Node root, NodeVisitor nodeVisitor, Map<Class<?>, Object> rootVars) {
Copy link
Member

Choose a reason for hiding this comment

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

I know the previous code did not have JavaDoc but can you please fix this up.

We now have a map of variables and hence we should explain them and how one might get access to them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly, I took a stab at writing the JavaDocs for the methods I understand. I am not sure how the parallel methods operate so I did not write JavaDocs for them.

@chriswr95
Copy link
Contributor Author

@bbakerman would you mind taking a second look at this?

Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Thank you

@bbakerman bbakerman merged commit dd1d500 into graphql-java:master Nov 12, 2021
@chriswr95 chriswr95 deleted the add_context_to_ast_transformer branch November 30, 2021 18:01
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.

3 participants