Added factories for SourcePersisters and the various GsonParser imple… #35
Conversation
…mentations. Updated README to use new factories. Updated example activity to use new factories. All factories return the general Store interfaces.
|
Thanks for contribution! Closes #30 |
| */ | ||
| public static <T> Parser<Reader, T> createReaderParser(Class<T> parsedClass) { | ||
| if (parsedClass == null) throw new IllegalArgumentException("parsedClass cannot be null."); | ||
| return new GsonReaderParser<>(new Gson(), parsedClass); |
There was a problem hiding this comment.
This is nice. Great addition!
There was a problem hiding this comment.
Maybe the gson instance should be a singleton. My thinking is why create more than one if multiple stores want to use the convenience method.
There was a problem hiding this comment.
Yeah I had the same idea.
Not sure if that's something you would want to provide in the library, or have them write themselves.
There was a problem hiding this comment.
@brianPlummer @alexio thoughts?
There was a problem hiding this comment.
The other trick is that the factory is all static and has no state.
I'm not a fan of static variables that are mutable.
There was a problem hiding this comment.
I would vote against maintaining a singleton, if multiple instances are an issue the caller can resolve by passing in its own.
|
Also, @digitalbuddha wasn't sure what the opinions on Precondition assertions were, as I didn't see any in the code besides the transcribed Guava cache code. That being said, my personal preference is that I'm always happy when a library code fails immediately and clearly (of course at the cost of some boilerplate in the library code). |
|
I agree I like precondition checks as well. Feel free to add |
|
Let's wait for 1 more collab review before merging but lgtm. I agree on not holding static state. |
|
I was also thinking about missing Preconditions when doing #28, but decided to not add any because there wasn't any in the codebase. Maybe we could create an issue for that so a single PR can be made to add preconditions for the whole library (where necessary). |
|
Sure ticket made #41 |
| return new GsonSourceParser<>(new Gson(), parsedClass); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Comment here and on next one seem to be reversed
| import okio.BufferedSource; | ||
|
|
||
| /** | ||
| * Created by Scott on 1/8/2017. |
There was a problem hiding this comment.
Replace with an appropriate javadoc.
There was a problem hiding this comment.
actually just remove it, we can go to github to see who made file.
There was a problem hiding this comment.
didn't mean to keep the "created" thing, just IDE setting I forgot to change, I'll fix.
|
remove the top level comment & flip the other two and this should be good to go. |
|
@scottmeschke looks good! strip out the "created by" at the top of SourcePersisterFactory.java and it should be good to merge in!! :) thank you so much!! |
|
at work now, I'll fix these issues at home tonight. ty for the review |
|
Fixed the javadoc mistakes. |
|
Thank you do much for the contribution! We haven't even setup checkstyle yet. Love this community 💯 |
scottroemeschke
deleted the
scottmeschke/source_persister_and_gson_parser_factories
branch
"Added factories for SourcePersisters and the various GsonParser implementations. Updated README to use new factories. Updated example activity to use new factories. All factories return the general Store interfaces."