1210 deferred aligned with apollo#1221
Conversation
This reverts commit 38dfab1
andimarek
left a comment
andimarek
left a comment
There was a problem hiding this comment.
see my comments .... @bbakerman great work!
|
|
||
| import java.util.List; | ||
|
|
||
| public abstract class DeferredDirectiveAbstractRule extends AbstractRule { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| import java.util.List; | ||
|
|
||
| public class DeferredDirectiveOnNonNullableField extends DeferredDirectiveAbstractRule { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| Directive directive = field.getDirective(Directives.DeferDirective.getName()); | ||
| if (directive != null) { | ||
| Map<String, Object> argumentValues = valuesResolver.getArgumentValues(Directives.DeferDirective.getArguments(), directive.getArguments(), variables); | ||
| return (Boolean) argumentValues.get("if"); |
There was a problem hiding this comment.
can this be null? => unboxing will cause NPE
There was a problem hiding this comment.
now made it non null as an arg declaration
| echo "Building on master" | ||
| BUILD_COMMAND="./gradlew clean assemble && ./gradlew check --info && ./gradlew bintrayUpload -x check --info" | ||
| fi | ||
|
|
There was a problem hiding this comment.
we must remember that must be changed again before merge!
fix SubscriberPublisher test: should throw NPE
andimarek
added
the
Not to be merged
|
I'm wondering what the status of this branch is? It's very important for my project (and seems to work well). Would you like help cleaning/documenting to get it merged? Just point me in the right direction 👍 |
|
@bbakerman your opinion on the current state of it? |
|
the implementation here looks good, but it is not fully tested and we will not merge it soon. |
|
There is a valuable feature because current deferred has a problem with matching paths. So, is any forecasts of that feature? We want to use it, and here is the question: wait for this PR will be merged or use this branch or maybe do the fork? Do the tests is the main problem with that branch(maybe the community can help)? And Merry Xmas! |
|
By now, I just downloaded rep. Checkout to that branch and merged it with v11.0 and uploaded it to my company's rep(so after all tests was excellent). That's an easy solution, but I hope that will be merged soon. |
|
@Me1kaa I can't give you a forecast when we will continue working on that, sorry. so yes: if it is urgent for you I would recommend to build this brand yourself for now. |
bbakerman
removed
the
Not to be merged
|
This is now ready to go |
4 participants
note: this is a second PR directly from this repo in order to deploy it while in development
See #1210