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

Create guicesupport module #48

Merged
merged 4 commits into from
Mar 14, 2018
Merged

Conversation

badgerwithagun
Copy link
Member

Works towards #14.

Moves the guicesupport package to a new Maven module, and migrates the JIT bindings for SqlScriptExecutor and TableLoaderBuilder to there, removing @Inject and @ImplementedBy.

This is broadly the same pattern we will follow to remove the Guice stuff elsewhere (effectively, change everything to Dagger style bindings). Will hack away at those as a series of future PRs.

@badgerwithagun
Copy link
Member Author

@wnicholson, are you able to review this one? Otherwise I can assign it elsewhere.

@wnicholson
Copy link
Member

Looks good to me - merging + two tiny tweaks.

@wnicholson wnicholson merged commit c2ccb99 into alfasoftware:master Mar 14, 2018
@badgerwithagun badgerwithagun deleted the guice branch March 14, 2018 16:05
badgerwithagun added a commit that referenced this pull request Mar 14, 2018
This needs more work to sustain compatibility with existing code.

This reverts commit c2ccb99, reversing
changes made to 8bedf83.
@badgerwithagun badgerwithagun restored the guice branch March 14, 2018 16:56
@badgerwithagun
Copy link
Member Author

This broke builds at Alfa and had to be reverted. The main problem appears to be various builds we have which assume there are JIT bindings in place. This either means that they blow up trying to replace said bindings, or simply assume the default bindings are in place.

@badgerwithagun
Copy link
Member Author

This PR is in a merged state so I'll submit a second one at some point.

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

Successfully merging this pull request may close these issues.

2 participants