search

HubSpot / dropwizard-guice

Adds support for Guice to Dropwizard
Apache License 2.0
266 stars 95 forks source link

Config #49

Open oillio opened 9 years ago

oillio commented 9 years ago

Add bindings for each field in the configuration object(s). See PR #46 for more discussion on this feature.

eliast commented 9 years ago

You should squash all of these commits into one.

http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

oillio commented 9 years ago

The problem is that this functionality depends on the double injection feature in PR #46. The new code for @Config is squashed down to the last commit in this branch. Once #46 is accepted most of these commits should disappear.

mrserverless commented 9 years ago

hi @oillio I think it may be possible in #46 to squash everything after bf0197755e0c682cbfe320270fe2bd6ddce2f817 because all the commits are authored by you.

having been a major culprit in making unnecessary commits, I've learned that after the commits are pushed, you can still squash by doing: reset followed by push -f. As per this answer: http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git/5201642#5201642

oillio commented 9 years ago

Fair enough. Done.

mrserverless commented 9 years ago

I haven't had a chance to trying this out yet, I'm wondering does @Config depend on the double injection change? I.e. if I use @LazySingleton to work around #19 instead of starting a double injection, I should still be able to use the @Config binding right? If that is the case, then it would make sense to split out the changes from #46 so that this can go in independent of #46.

oillio commented 9 years ago

Sure, the @Config functionality could be implemented without double injection. The way the code is currently implemented, it depends on double injection. And I believe it is the cleanest way to implement it. If implemented with a lazy setup, the user has to be careful to ensure that any use of @Config data is not done too soon (by use of @LazySingleton for instance). Technically this is true for any implementation, but it is easier on the user with double injection; the Guice modules that may use @Config data aren't loaded into the injector until after the data is avaliable. Tricks like @LazySingleton and careful use of Providers are not required.

I don't really want to take the time to implement @Config in a different and inferior way, if double injection is going to be merged soon. I still have hope double injection will be approved any day now. It solves issues that have been brought up a number of times, it has only minor impact on performance or usability, the 0.8 upgrade is the ideal time to release such a change, and the HubSpot guys have not yet expressed any issues with the design.

alexan commented 9 years ago

is there a reason why this don't get merged?