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

Fixing issue 362 and improvement #363

Merged
merged 5 commits into from Aug 29, 2019
Merged

Fixing issue 362 and improvement #363

merged 5 commits into from Aug 29, 2019

Conversation

dlemures
Copy link
Collaborator

@dlemures dlemures commented Aug 29, 2019

This PR contains the following changes:

  • Solving issue 362. In order to fix the problem, it moves KTP binding API to use classes instead of extension functions.
  • Cleaning KTP class (no need to have companion object or extend Toothpick) and moving injector replacement logic to different class (single responsibility). Also it will not make it possible to write (the suggested way by the IDE...):
KTP.TP.openScope...
  • Trigger member injection (annotated fields and annotated) even when delegates are used. This will allow to use method injection on roots using KTP.

These changes are improvements that do not change the public API.

@coveralls
Copy link

coveralls commented Aug 29, 2019

Coverage Status

Coverage remained the same at 95.645% when pulling 1ff78ca on dlemures/362 into 211a0d9 on master.

/**
* Replaces Toothpick injector to be able to notify delegates.
*/
object InjectorReplacer {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KTPInjector

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just keep the method or create a proper subclass r.object only for the injector of KTP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed.

I tried to use an extension functions from within the same package, but it doesnt work cause of the visibility. :(

}
}
init {
InjectorReplacer.replace()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do the assignment here and use a dedicated objwct

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot do it cause of visiblitiy. Otherwise we would need to make the field protected (instead of package-private) and extend Toothpick (which means, having to go back to companion and KTP.TP....)

/**
* Replaces Toothpick injector to be able to notify delegates.
*/
object InjectorReplacer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this internal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@@ -39,7 +39,7 @@
// its transformation
private static final ConcurrentHashMap<Object, Scope> MAP_KEY_TO_SCOPE =
new ConcurrentHashMap<>();
protected static Injector injector = new InjectorImpl();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better package-private

@dlemures dlemures force-pushed the dlemures/362 branch 2 times, most recently from 5cacbc9 to 8ce314c Compare August 29, 2019 16:41
@stephanenicolas
Copy link
Owner

RTM. Thx a lot. Good changes ;)

@dlemures dlemures merged commit 6dbd851 into master Aug 29, 2019
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.

None yet

4 participants