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

Remove discovery dependency, rely on puli directly #40

Closed
sagikazarmark opened this issue Feb 5, 2016 · 9 comments
Closed

Remove discovery dependency, rely on puli directly #40

sagikazarmark opened this issue Feb 5, 2016 · 9 comments

Comments

@sagikazarmark
Copy link
Member

Discovery is a static, convenience wrapper around Puli discovery. Instead of using discovery as factories for services, we should use Puli discovery service to find resources at compile time (if that's possible at all).

This would have two side-effects:

  • If puli discovery is used at compile time, then container compilation will fail if no resource is found (message factory, etc). IMO this is good, because the earlier we know something is bad the better it is
  • We won't get the same NotFound exception

WDYT?

@dbu
Copy link
Collaborator

dbu commented Feb 8, 2016

agreed. did you want to do a PR for that?

@sagikazarmark
Copy link
Member Author

Yeah, I wanted, but I didn't have the time. Go for it if you have.

@dbu
Copy link
Collaborator

dbu commented Feb 8, 2016 via email

@sagikazarmark
Copy link
Member Author

/cc @webmozart

@webmozart
Copy link

@dbu @sagikazarmark The original goal was to release a first stable release by end of 2015. Unfortunately, we failed to achieve that. Our plan is to simplify Puli in order to prepare it for a final release. I hope we can do it by mid 2016.

@sagikazarmark
Copy link
Member Author

Thanks for the info @webmozart

@sagikazarmark
Copy link
Member Author

Hm, further thinking about it: I think we should refactor the discovery layer as well. Currently the whole discovery layer is static. We could make the discovery layer itself non-static to accept a Puli discovery instance.

But we need static, so we can add another layer around discovery which gets the puli factory and injects the Puli discovery into our discovery layer.

Basically I would do the same non-static thing inside the bundle. This way we have it in the discover (allowed to be reused with pure Puli), we have the static layer, and we reuse the code in the bundle as well.

@dbu
Copy link
Collaborator

dbu commented Feb 28, 2016

should we consider this a blocker for the 1.0 release or do we go with what we have and get this out? i would prefer releasing now - there is a lot of refactoring involved and puli itself is not released as stable yet anyways - so likely we have to adapt things again anyways and do a 2.0 release later this year.

@sagikazarmark
Copy link
Member Author

Well, I am fine with that. However, currently discovery is actually used in this package. I would still prefer rewritting it's usage in this bundle so that we can remove the discovery dependency and use directly the puli discovery service provided by the puli bundle.

sagikazarmark added a commit that referenced this issue Mar 1, 2016
POC how to not be dependent on discovery

Fix unit tests

Fix factory method

Remove duplicated service

Move discovery to compiler pass

Applied fixes from StyleCI

Fix puli executable

Use class constant

Fix puli command order

Fix lowest dependency issue

Fix HHVM tests

Improve tests
sagikazarmark added a commit that referenced this issue Mar 1, 2016
POC how to not be dependent on discovery

Fix unit tests

Fix factory method

Remove duplicated service

Move discovery to compiler pass

Applied fixes from StyleCI

Fix puli executable

Use class constant

Fix puli command order

Fix lowest dependency issue

Fix HHVM tests

Improve tests
@Nyholm Nyholm closed this as completed in 4463f6c Mar 1, 2016
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

No branches or pull requests

3 participants