ResponseMapFactory #3857 #3894
ResponseMapFactory #3857 #3894
Conversation
58cdde1 to
bf510c2
Compare
|
Thanks for the PR @dfa1. In general we prefer not to open up Can we instead define an abstract Factory method or so that can be provided via config, similar to |
|
@andimarek sounds good... I have several concrete classes deriving from What could be a good name? |
69c9ee7 to
a68fee8
Compare
Let's go with ResponseMapFactory for now. |
a68fee8 to
8f07b72
Compare
|
@andimarek done... please have another look and feel free to suggest better javadoc ;) |
a5cfbd8 to
fca1f2c
Compare
|
@andimarek @bbakerman I just rebased to fix some conflicts, do you like the current state? do you expect more javadoc? |
| */ | ||
| ResponseMapFactory DEFAULT = new DefaultResponseMapFactory(); | ||
|
|
||
| Map<String, Object> create(List<String> fieldNames, List<Object> results); |
There was a problem hiding this comment.
I feel like this should be generic
<K,V> Map<K, V> create(List<K> fieldNames, List<V> results);
Also naming matters
The map we give back is insertion ordered. This is important at times
So I think the create method should be createInsertionOrdered(...)
There was a problem hiding this comment.
List<String> fieldNames, List<Object> results
the names here should be generic
List<String> keys, List<Object> values
There was a problem hiding this comment.
at some time in the future we might be using this to make more "maps" in different places say
There was a problem hiding this comment.
thanks for the feedback! I covered all the points but one: I left out the generics because it is in contrast with a comment from @andimarek.
I do see 2 alternatives:
- use
MapFactoryand have the generics - use
ResponseMapFactoryand drop the generics (as they will always K=String and V=Object)
it is acceptance to keep ResponseMapFactory without generics by now?
This will allows to customize which java.util.Map concrete class to use. For example, eclipse-collections has Map implementations with better memory footprint than JDK maps.
fca1f2c to
bff9b62
Compare
|
@andimarek @bbakerman hi guys, this PR is ready to be reviewed... or do you see something else missing? |
|
I am happy to take this PR in shape HOWEVER I dont think we should wire the MapResponseFactory into the graphql object This is unsual config - we dont expect people to use this very often. As such I think we should wire this into play via the new If we had out time again I would do this for ValueUnboxer as well but that is a past decision and this is a present once So while I support the idea of a MapResponseFactory - it is not usual to expect this to be set and its clutters the API. Thoughts @andimarek ? |
|
I agree with @bbakerman here .. perfect for our very new unusual config area. second question @dfa1 : does it need to be a function doing so much? could it be a just a constructor/factory function? |
|
@dfa1 - we are likely to merge this PR and then change so that This fits in with our new "unusual configutation" mechanism AND it declutters the public common API |
|
thanks @bbakerman @andimarek ! 💯
@andimarek : yes, also a factory function is fine
@bbakerman in our projects, all requests will use same We are using ... of course, I'm going to our code to use the "unusual configuration" mechanism you're proposing ;) |
Proposal for #3857. I would like to write a unit test too: the idea is to make sure that all nested maps are really built with the customization.