symfony-cmf / routing-auto

RoutingAuto component
https://cmf.symfony.com
Other
6 stars 11 forks source link

Changed component file structure and added script to test bundle integration #4

Closed dantleech closed 10 years ago

dantleech commented 10 years ago

This PR:

so much for atomic PRs ..

wouterj commented 10 years ago

I'm sorry, but I'm still -1 for this. I think it's way easier to move the functional tests to this bundle.

Furthermore, the src, test and vendor directories are not common in the complete Symfony environment. Non component nor bundle uses it.

It also means we have to add 2 namespaces to Composer, making the autoloading in production slower to support testing in the dev environment.

dantleech commented 10 years ago

For the tests. I think do want to move them here. But I am just slightly scared to venture into that unknown territory.. but OK lets try it. Sure we could have done it alreadumy with the time already spent doibg it this way.

For the structure - for me it is much cleaner - it doesn't mix meta files and source files - Bundles grt away with it more because they only have one source file in the root dir.

Symfony is packaged like this - sf components are not because they are subtree splits.

Otherwise I think most other Components are packaged in the way I propose.

I can't see the autoloading compromising speed - although we could benchmark it.

Opinions? @dbu @lsmith77 ?

Wouter J notifications@github.com wrote:

I'm sorry, but I'm still -1 for this. I think it's way easier to move the functional tests to this bundle.

Furthermore, the src, test and vendor directories are not common in the complete Symfony environment. Non component nor bundle uses it.

It also means we have to add 2 namespaces to Composer, making the autoloading in production slower to support testing in the dev environment.


Reply to this email directly or view it on GitHub: https://github.com/symfony-cmf/RoutingAuto/pull/4#issuecomment-47401363

Sent from my Android device with K-9 Mail. Please excuse my brevity.

dantleech commented 10 years ago

I was thinking today, and remembered the reasons I didn't want to do the functional testing here:

This is literally what CI is for - continuous integration. We are continuously integrating this component into the Bundle. I that makes sense - even if we do add functional tests here.

wouterj commented 10 years ago

CI is Continuous integration of development tools (like testing, linting, etc.) used for the package, it's not about the integration of the package into a PHP project.

I've a new solution, because I don't think we will agree on one of the 2 current proposed solutions: Put everything back in RoutingAutoBundle and use git subtree splits to create a read only RoutingAuto repository containing the component. This way, development, testing and CI all happends at the same place, but the main logic can still be used standalone.

dantleech commented 10 years ago

No .. the component needs to be in the Component namespace, this is why I am doing this now.

I really do not see a problem with bootstrapping the RoutingAutoBundle for testing the integration. This can be a short term solution with no BC problems. We can write Integration tests later at our own leisure.

On 29/06/2014, Wouter J notifications@github.com wrote:

CI is Continuous integration of development tools (like testing, linting, etc.) used for the package, it's not about the integration of the package into a PHP project.

I've a new solution, because I don't think we will agree on one of the 2 current proposed solutions: Put everything back in RoutingAutoBundle and use git subtree splits to create a read only RoutingAuto repository containing the component. This way, development, testing and CI all happends at the same place, but the main logic can still be used standalone.


Reply to this email directly or view it on GitHub: https://github.com/symfony-cmf/RoutingAuto/pull/4#issuecomment-47455867

Dan Leech

Web Developer www.dantleech.com

dantleech commented 10 years ago

For the record, if I think about moving the functional tests here it raises more problems, for example, we have phpcr-odm - fine. But then maybe we have couch-db and propel.

Do we really want a component to pull in all these things for the functional tests? And secondly should a component really have third-party integrations? But thats another dicsussion.

The same problem occurs in Bundles of course, but a component should be leaner. Ultimately this is the case for pushing code that binds the component to specific backends into separate packages.

But then it would be nice to have a CI system which tests any changes made to the component (or the bundle) against all the backends - using an approach similar to what I suggest here.

So in short I am going in circles and coming back to this solution.

dantleech commented 10 years ago

@WouterJ I would like to merge this - as I said before it doesn't bind us to this solution but it does enable us to go forward.

wouterj commented 10 years ago

@dantleech assume the Bundle has a bug and the Component doesn't. In such cases, the Component should be stable and have a passing build and the Bundle should be unstable with a failing build. Using this way, both are unstable and have failing builds. The builds no longer tell you information about the package itself, but also about other packages.

Having the Component rely on the tests of the bundle means that the component is too coupled to the bundle.

wouterj commented 10 years ago

And I honestly don't see why a bundle can rely on many implementations for testing, but a component can't...

At last, the tests are still failing for the same reason as before this PR.

dantleech commented 10 years ago
  1. Hmm. Yeah I see your point with the coupling, that is bad. I guess that sort of test would be better running as another job somewhere.
  2. Re. many implementations -- Sorry, my opinion doesn't really apply to this PR. I don't think a bundle should have many implementations either, like I said, its another discussion, I will blog about it or something.
  3. The failing tests are because master has a dependency on itself. This PR will probably need some firefighting if this is merged.

Argh. I do see your point. How tedious. I am still rather on the side of merging this now. But that would also mean that we are +1 for the directory structure change. Maybe we should just drop this whole PR and concentrate on functional tests and I will just have to accept that as the lesser of two evils.

Are you IRC tonight?

wouterj commented 10 years ago

@dantleech I'm on IRC till 8.30 PM tonight.

dantleech commented 10 years ago

I think we decided to not do this.