Add DI, remove static code and many code duplications, add DI usage e… #71
Conversation
| */ | ||
| public class Main { | ||
| public static void main(String[] args) throws IOException, MandrillApiError { | ||
| final ClassPathXmlApplicationContext context = new ClassPathXmlApplicationContext("classpath:applicationContext-LutungMandrill-all.xml"); |
There was a problem hiding this comment.
I kind of disagree adding a Main class to the repo to show how to "wire" it all up together. I think if you want to have an example, you can better have this as a part of the README.
There was a problem hiding this comment.
Sure, makes sense. I'll move it.
|
We are not going to make this library depend on Guava, Spring core and Spring context. Not everyone uses DI and those who do not everyone uses Spring. What exactly are you trying to solve here? The only issue I see this resolving is the ability to pass in the mandrill root url which was requested once before. If you want the ability to proxy to some other server we can expose a constructor where you pass in a |
|
Maybe that's correct, it shouldn't actually depend on Spring core/Context it was depending for it for the Main class to run I guess, but the abundance of static methods make it hard for people to change the internals so at least that I see as a good thing. The guava is more or less meh for me. |
|
I fixed most of the stuff and removed the spring dep (it was a bad idea - I agree). |
|
I'd also remove the lutung-mandrill-all.xml in the resources folder. I think everybody knows how to wire their stuff, the important thing is removing the statics. |
| @Override | ||
| public void accept(final QueryExecutor queryExecutor) { | ||
| addContent(templateContent, queryExecutor); | ||
| addContent(mergeVars, queryExecutor); |
There was a problem hiding this comment.
addContent(mergeVars, queryExecutor) will overwrite "template_contents" set by executing addContent(templateContent, queryExecutor)
| @@ -55,11 +52,9 @@ public MandrillExportJobInfo info(final String id) | |||
| */ | |||
| public MandrillExportJobInfo list() | |||
| @@ -33,12 +32,9 @@ public MandrillInboundApi(final String key) { | |||
| */ | |||
| MandrillInboundDomain[] domains() | |||
There was a problem hiding this comment.
all the calls are package-private, perhaps should be public?
…xample