Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config #49

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Config #49

wants to merge 9 commits into from

Conversation

oillio
Copy link
Contributor

@oillio oillio commented Feb 26, 2015

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

@eliast
Copy link
Contributor

eliast commented Feb 26, 2015

You should squash all of these commits into one.

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

@oillio
Copy link
Contributor Author

oillio commented Feb 26, 2015

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
Copy link
Contributor

hi @oillio I think it may be possible in #46 to squash everything after bf01977 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

Break out configuration code.
Implement InjectedCommands.
Added tests.
Various bug fixes and refactorings.
@oillio
Copy link
Contributor Author

oillio commented Mar 2, 2015

Fair enough. Done.

@mrserverless
Copy link
Contributor

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
Copy link
Contributor Author

oillio commented Mar 4, 2015

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.

Dan Jasek added 2 commits March 4, 2015 09:42
Conflicts:
	.gitignore
	README.md
	pom.xml
	src/main/java/com/hubspot/dropwizard/guice/AutoConfig.java
	src/main/java/com/hubspot/dropwizard/guice/GuiceBundle.java
	src/test/java/com/hubspot/dropwizard/guice/AutoConfigTest.java
	src/test/java/com/hubspot/dropwizard/guice/GuiceBundleTest.java
	src/test/java/com/hubspot/dropwizard/guice/objects/ExplicitResource.java
	src/test/java/com/hubspot/dropwizard/guice/objects/InjectedTask.java
@alexan
Copy link

alexan commented Sep 4, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants