search

square / Cleanse

Lightweight Swift Dependency Injection Framework
Other
1.78k stars 90 forks source link

Cleanse Service Provider Interface (aka Plugins) #120

Closed sebastianv1 closed 4 years ago

sebastianv1 commented 4 years ago
holmes commented 4 years ago

Looks good, but why not convert the ValidationVisitor to a CleanseBindingPlugin to prove that it works?

sebastianv1 commented 4 years ago

Could be an option, but I do like that it's part of the core framework and not just a plugin. Could be worth migrating one day.

holmes commented 4 years ago

It'd be nice to have something as a proof of concept that this works. If you don't want to convert that one I'd strongly encourage you to create one in the demo.

A working example is the most likely way to get people to build on this feature.

On Thu, Nov 14, 2019, 08:54 Sebastian Shanus notifications@github.com wrote:

Could be an option, but I do like that it's part of the core framework and not just a plugin. Could be worth migrating one day.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/square/Cleanse/pull/120?email_source=notifications&email_token=AAAOOGWHWDPFBFFJLFVD5YDQTV7FNA5CNFSM4JMJ7RQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEECQVUY#issuecomment-553978579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAOOGQCCSTWFP43FFEXJ5TQTV7FNANCNFSM4JMJ7RQA .

sebastianv1 commented 4 years ago

That's a good point. I'll work on an example impl with some tests.

sebastianv1 commented 4 years ago

@holmes I've added a simple unit test. My thinking is to create a separate framework for the plugin samples that can be consumed independently of Cleanse. This would involve adding a new podspec (named CleansePlugins for instance) that would depend on Cleanse. In order for this to happen, I'd rather land this and have CleansePlugins point at the master branch of Cleanse instead of this development branch. Overall, it makes more sense as a separate PR after this is merged into master.