WIP - Ability to get parsed directive argument values inside a DataFetcher#1229
WIP - Ability to get parsed directive argument values inside a DataFetcher#1229kaqqao wants to merge 1 commit into
Conversation
| */ | ||
| <T> T getArgument(String name); | ||
|
|
||
| Map<String, Map<String, Object>> getDirectiveArguments(); |
|
|
||
| Map<String, Object> getDirectiveArguments(String directiveName); | ||
|
|
||
| <T> Optional<T> getDirectiveArgument(String directiveName, String argumentName); |
There was a problem hiding this comment.
As much as Optional is a better pattern, its mixing styles on this class. We already use null via getArgument() (because it predates Java 8) so I think we should stay consistent to the other methods in this interface
There was a problem hiding this comment.
The difference between this and getArgument(argumentName) is that in this case even the directiveName might be wrong, before we even check the argumentName. But I get your point., it does look weird.
| } | ||
| return valuesResolver.getArgumentValues(fieldVisibility, directive.getArguments(), dir.getArguments(), executionContext.getVariables()); | ||
| })); | ||
|
|
There was a problem hiding this comment.
Can we make put this into a DirectivesUtil helper class so its simpler to read and easier to test
Map<String, Map<String, Object>> directiveArgumentValues = DirectivesUtil.readDirectiveValues(field)
See graphql.DirectivesUtil
There was a problem hiding this comment.
Do you think it would maybe be better to create a new type to represent this or are you ok with Map<String, Map<String, Object>>?
There was a problem hiding this comment.
I think this should be a Map<String,graphql.schema.GraphQLDirective>
That is a field has a map of named graphql.schema.GraphQLDirective objects. From that you can get the arguments and the values to this arguments.
We have other code somewhere (off hand) that helps create that
|
Ahh sorry about the comments - didnt see the WIP bit |
|
No worries, they're still legit comments :) |
andimarek
added
the
Not to be merged
|
@kaqqao what is the status of that? thanks |
|
Terribly sorry for the trouble, but I didn't get a chance to finish this 😩 |
|
@kaqqao no worries :-) |
|
This is not longer needed and has been re-implemented via #395 |
3 participants
Closes #1228
This is just an initial implementation to exemplify what I have in mind.
I didn't pay attention to code duplication, missing tests etc which I'd of course fix before merging this.