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

Separate drivers to smaller packages #256

Open
6 of 9 tasks
sagikazarmark opened this issue Oct 27, 2016 · 37 comments
Open
6 of 9 tasks

Separate drivers to smaller packages #256

sagikazarmark opened this issue Oct 27, 2016 · 37 comments

Comments

@sagikazarmark
Copy link
Contributor

sagikazarmark commented Oct 27, 2016

Drivers to split:

  • AMQP
  • Pheanstalk
  • Redis/Predis
  • Doctrine
  • Iron
  • SQS
  • Mongo
  • AppEngine
  • QueueInterop

FlatFile does not rely on any external dependencies, also it's a sane default for playing around.


Initial comment:

Although I love the idea of having one single package for everything, I would consider having smaller packages for the drivers with actual dependencies. For example: pheanstalk-driver with a dependency on pda/pheanstalk instead of suggestion.

It would add more maintenance, but would make the main package a bit cleaner. WDYT?

I am willing to work on this one if you agree.

@henrikbjorn
Copy link
Contributor

Have always been the idea.

@sagikazarmark
Copy link
Contributor Author

Alright...should I start by creating a repo in my namespace, transfering it and opening a PR for removal in the main one?

@henrikbjorn
Copy link
Contributor

Not sure what the right approach is, if we want to do like Symfony and have sub tree splits.

@sagikazarmark
Copy link
Contributor Author

Well, we are basically talking about one or two classes per repo, so I am not sure it worth to have subtree splits. It requires special versioning strategy, and so on. We discussed this approach for HTTPlug and decided not doing it for HTTP clients and adapters, since they might evolve separately. Also subtree splitting makes dependency management a bit harder, so I am against it for a project like this.

@henrikbjorn
Copy link
Contributor

I can create the repos and then ill give you write rights to them. is that ok ?

@sagikazarmark
Copy link
Contributor Author

sagikazarmark commented Oct 28, 2016

Sounds great.

Some more questions to clear:

  1. Namespacing: Should it be Bernard\Driver or Bernard\[DriverName]? eg. Bernard\Driver, class name Pheanstalk or Bernard\Pheanstalk, class name PheanstalkDriver or maybe Bernard[\Driver]\Pheanstalk, class name Driver (at HTTPlug we have names like Http\Client\Guzzle6 namespace, class name: Client)?
  2. Package names: bernard/[driver_name]-driver?
  3. Is it ok, if I write spec tests immediately for behaviour, unit tests for the rest?
  4. Are you going to setup travis and packagist as well or I should (I think I cannot setup travis without organizational rights)?

@sagikazarmark
Copy link
Contributor Author

Just realized this is a duplicate of #92

@sagikazarmark
Copy link
Contributor Author

My vote goes to:

Repo: bernardphp/[driver_name]-driver

Namespace: Bernard\Driver\[DriverName]

Driver classname: Driver (+any other driver specific classes, like schema for doctrine)

Package name: bernard/[driver_name]-driver

If it's okay for you, I would create a boilerplate-driver repo/package first as a package boilerplate which then can be used later.

@kynx
Copy link
Contributor

kynx commented Oct 28, 2016

Lurker here would go for Bernard\Driver\PheanstalkDriver every time - less chance I'll have to waste time aliasing it.

@sagikazarmark
Copy link
Contributor Author

Although it's simple, it has a few drawbacks:

  • Bernard\Driver would be the namespace for autoloading and it's not really a good idea to have the same namespace for multiple directories
  • If you need additional files for a driver (like in case of doctrine) it seems to be wrong to have it in a driver namespace

Also, if you import Bernard\Driver\[DriverName] namespace, you can use [DriverName]\Driver class.

@sagikazarmark
Copy link
Contributor Author

Ping :)

@holtkamp
Copy link
Contributor

Well, we are basically talking about one or two classes per repo, so I am not sure it worth to have subtree splits.

Funny, read this yesterday. Today on twitter: https://twitter.com/TobiasNyholm/status/810882518572564480

@sagikazarmark
Copy link
Contributor Author

SubtreeSplit is a nice approach when you don't want to use the master repository or you want all from them. In case of an adapter-pattern like project I am not sure if it adds too much, but @Nyholm might have a few interesting thoughts here since he maintains two projects like that.

@Nyholm
Copy link

Nyholm commented Dec 20, 2016

For the users:
A subtree split is a good idea for making smaller packages. It will remove the need for optional dependencies and you can version packages independent of each other. Ie if one driver have a BC break you do not have to bump all adapters.

For the maintainers:
I find subtree split a good approach when you share code between adapters. (Like PHP-cache.com). This organization is only sharing an interface which means maintainers will not see any major benefits, but then again, there is no drawback either.

Tests:
In your case you do not have to do anything special here.

Namespace:
Im not expert here. But doing Bernard\Driver\[DriverName]\Driver seams reasonable.


[Drivers] might evolve separately. Also subtree splitting makes dependency management a bit harder, so I am against it for a project like this.

I do not understand this argument. I think you mean that if we do like Symfony does an keep the same version on each driver, then yes. I agree. But if one configure the subtree split not to push tags then I do not think this is an issue.

I do not like to keep all the drivers in the same package because of the reasons above. I would go with a subtree split over completely separate repos since it is now way easier to get started with thanks to subtreesplit.com

@sagikazarmark
Copy link
Contributor Author

I think you mean that if we do like Symfony does an keep the same version on each driver, then yes. I agree.

Yes

I think the main argument is that we have a central package here, which should hold some common code, and adapters/drivers should require it. From this point of view subtree split does not seem to be a good idea to me. Maybe having a separate drivers package would be better? And apply subtree split there? Would that be better?

@Nyholm
Copy link

Nyholm commented Dec 20, 2016

Ah okey. The master repo https://github.com/bernardphp/bernard should not be a package you can download. It should only be for development.

If you want to keep the package bernard/bernard point to this repo you cannot have a subtree split form here. You have two options:

A)
Create a slave repo and point the package bernard/bernard to that new repo. And create slave repos and new packages for each driver. Now you may do a subtree split for this repo

B)
Create a new bernardphp/drivers repo and do a subtree split to new slaves.

@sagikazarmark
Copy link
Contributor Author

Yeah, option B what I meant. That would actually be a good idea then, but somehow destroys the idea of subtree split, doesn't it?

@Nyholm
Copy link

Nyholm commented Dec 28, 2016

That is what I am trying with https://github.com/php-translation/platform-adapter Adapters do not share anything and there is no shared code apart from the adapters in the master repository.

Having this setup does not bring many benefits. Just that it is nice if Im going to change many adapters at once and there are lot less repositories to supervise. I may include php-translation/common in this master repo since it is a shared dependency for all adapters.

So.. choosing option B is great because you get small packages, but a subtree split is barley needed.

@sagikazarmark
Copy link
Contributor Author

I would like to pick this issue up and move drivers to a separate repo using subtree split. I will do some research and write down the plan here.

@acrobat
Copy link
Member

acrobat commented Jun 23, 2017

Maybe we can use https://www.subtreesplit.com/ from @Nyholm for the subtree split management. But indeed good idea to pick up this issue!

@sagikazarmark
Copy link
Contributor Author

That's the plan, yes.

@kynx
Copy link
Contributor

kynx commented Jun 23, 2017

+1

@acrobat
Copy link
Member

acrobat commented Jul 25, 2017

@sagikazarmark what are the exact steps to do this? Do we just create the separate repo's by hand and move the code and then initialize subtreesplit or will the code moving part be done by the subtreesplit?

This way we can already do some preparations for the move!

@Nyholm
Copy link

Nyholm commented Jul 25, 2017

You need to create separate repos and the tell subtreesplit what folder should be pushed to which repo.
You do not need to populate the new repos yourself.

@sagikazarmark
Copy link
Contributor Author

sagikazarmark commented Jul 25, 2017

So the plan is to create a separate drivers repo (already done) and move current drivers there.

Class name will be Bernard\Driver\[DriverName]\Driver (eg. Bernard\Driver\Pheanstalk\Driver) which is somewhat inconvenient, but easy to maintain and documentation should resolve the convenience issue.

Repo names: [drivername]-driver
Package names: bernard/[drivername]-driver

It worked quite well with php-http

We also need to improve the test suite a bit. I would like to create a unified integration test suite (if possible) and use the same set of tests for each driver, even if it is only a small set of tests. Each driver can have it's own set of tests as well. Also, tests needs to be somewhat separated based on the featureset (eg. not all queue backends support peeking, etc).

I would like to create a POC driver first (probably Pheanstalk), work on the test suite, set up subtree split, etc. If it works well, we can migrate the rest of the drivers.

@sagikazarmark
Copy link
Contributor Author

Decided to go with an in-memory driver instead.

@sagikazarmark
Copy link
Contributor Author

@Nyholm what's the current best practice: run the tests when pushing to the main repo or when subtree is done?

(The first one sounds better to me)

@Nyholm
Copy link

Nyholm commented Jul 25, 2017

Yes. First one.

I also run Travis on individual driver to get the code coverage stats.

Have a look at php geocoder for a good Travis.yml

@acrobat
Copy link
Member

acrobat commented Jul 26, 2017

Thanks for the info @sagikazarmark! I will let you do the setup on the first driver to get all things right and then I will jump in and also do some drivers!

@acrobat acrobat mentioned this issue Sep 15, 2017
@acrobat
Copy link
Member

acrobat commented Sep 23, 2017

@sagikazarmark did you make any progress on the first driver? Or is there something I can do to start on this?

@sagikazarmark
Copy link
Contributor Author

Yeah, I started to get my head around this, have some tests standardized locally. Will send some branches for review over the weekend.

@acrobat
Copy link
Member

acrobat commented Sep 24, 2017

Ok great!

@namoscato
Copy link

I would be happy to help out with this as well.

@acrobat
Copy link
Member

acrobat commented Nov 5, 2017

Sorry to ping you again @sagikazarmark! Did you make any progress on this? Or can and somehow start some work on this (a single driver to test the whole implementation)?

@sagikazarmark
Copy link
Contributor Author

I created #337 with an initial in memory driver implementation. I gave having a core test suite much thought and I realized that without ugly hacks we can't really have an interface for setting driver state, so as for tests we should just have a sane template to follow.

I'm also thinking about writing spec tests instead of/additional to unit tests.

@sagikazarmark
Copy link
Contributor Author

I will also try to send a PR to the drivers package to have some ideas how migration would work.

@acrobat
Copy link
Member

acrobat commented Nov 5, 2017

Thanks will check it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release preparations
  
In progress
Development

No branches or pull requests

7 participants