-
Notifications
You must be signed in to change notification settings - Fork 50
missing for stable release #38
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
Comments
imo #26 can also be done after 1.0 is released. we can improve the toolbar even as patch release i think. |
In my opinion we should ask ourselves when to release a stable release. A bundle is kind of different from a a library. We could release when we are sure we do not break BC. Or we could release when we are somewhat feature complete and are not under heavy development. In terms of BC. What are our BC promise for this bundle? It does not have any public classes so our BC promise is just config? (Am I correct?) In either way we need to add tests and make sure it works on Travis with all the supported Symfony versions. |
i think we should not aim for feature completeness. the bundle can already do useful things, and people starting to use httplug in symfony should be able to use a bundle rather than define their own things (thus re-inventing the bundle). i think indeed the BC is for the configuration, so we need to be sure it makes sense and have enough tests to detect when doing non-BC changes to the configuration. and we should have some tests to know things work on all versions of symfony we claim to support. thought that looks ok except for https://github.com/php-http/HttplugBundle/blob/master/.travis.yml#L33 => we should not allow the lowest build to fail, that is strange. |
with #52 done, is there anything that prevents us from tagging a stable release? |
Like said before: Our BC promise is our configuration. PR #44 made a BC breaking change. Are we happy with how the configuration looks now? I think we need to document the bundle properly before we make a release. The current documentation is just a bunch of unstructured examples (yes, mostly my fault). I do also believe that we might find some quirks while organising the documentation that maybe should be resolved before tagging 1.0. I do also think we should fix #14 before 1.0. |
I would also do as explained in #40, even if we don't wait for a stable release of Puli. |
I just come to think about this: We should not do a stable release while we have dependencies that are not stable. If that is Puli or our own discovery package does not matter. |
Yeah, I know. I was kind of answering that comment here to keep the "When to release 1.0"-discussion in this thread instead. |
As I stated in that issue as well, we should remove the discovery package usage from the bundle entirely and use only Puli. If you take a look at the discovery package now, it is mainly a static wrapper around Puli. However, Puli provides discovery as a service in its bundle. So I propose getting rid of the static stuff and use Puli bundle and discovery service in this bundle directly. Expect a PR tommorrow. |
Cool, but that does not solve the problem with non-stable dependencies. If we remove dependency on We could, however, remove |
No, it does not solve the stability question, but we don't have to tag discovery 1.0 that way. And we cannot make puli bundle optional since the service is necessary. Container compilation will fail otherwise. But I don't think making it a dependency is a problem: it does not require the composer plugin and based on what's inside the bundle, it is very unlikely that any BC incompatible change will happen with it. So I don't see an issue in relying on it in our stable version. |
That's right. We can do 0.x versions until all our dependencies are stable.
Im not sure this is correct. You can configure HttplugBundle to not use Puli or any discovery. I believe it is a good idea to not require |
The problem I see with not requireing is the following: in order to use puli, we need a wrapper service. To goal is to find exactly one class of type, while Puli finds them all by default. This is why we need the wrapper. Currently static factories are registered as factories for default services. So if we do as we plan, we have to replace the discoveries with our custom wrapper service. If we don't require the bundle, we won't have the puli discovery service. If we don't have the discovery service, we can't register our wrapper service. If we don't have our wrapper service, we cannot register is as factory for default services. If we don't have default services, any other service that rely on them will fail. Since the actual discovery does not happen during compile time (also, default services can be overriden), we won't know whether we need the default discovery and the wrapper service or not. We also won't know if there is anything to guess or not. These will happen during runtime and we have to handle these cases runtime. Correct me if I am wrong, but I don't see how we could not require the puli bundle. |
I do not disagree with you on how to implement the discovery. But I do think we can know (before compiling the container) if the discovery is going to be used or not. I added a proof of concept PR (#54). |
So back to the main question: When do we do 1.0?
The work in #55 will remove our unstable dependencies and #57 will remove the documentation from this repo. Are we ready for a release as soon as those PRs are merged and new documentation is on the website? |
From my point of view: yes. Although the documentation needs improvement, we can tag 1.0 once it is removed from here and improve the documentation after. |
Awesome! It depends how comfortable we are with our configuration..
When we put everything together we might think: "Wtf, this does not make sense". |
I haven't followed the changed lately, so I leave this to you. ;) |
Im closing this in favor for #64 |
lets collect what remains to be done and discuss here whether things must be in 1.0.0 or not. lets create issues for concrete tasks and reference them, and put them into https://github.com/php-http/HttplugBundle/milestones/1.0.0 if we think we need them.
The text was updated successfully, but these errors were encountered: