Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

remove unused type attribute in Core::Role::Engine #149

wants to merge 1 commit into


None yet
5 participants

ghost commented Jan 4, 2013

The type attribute is unused, so I removed it.


celogeek commented Jan 5, 2013

This is not used now, but have a type could be usefull for plugins ?

Like Template plugins, or Logger plugin or Serializer plugin. And we can use that to do stuff.

Of course it seems useless now, but it could be helpfull for plugins.


ghost commented Jan 6, 2013

But what does it mean and what is it used for? Until that is defined, it shouldn't be in the code base.

There's no limitation on what can go in there, so if plugins start populating it with who-knows-what, and then semantics are attached to it, things might break.


celogeek commented Jan 7, 2013

Well, better to have a clear and sufficiant code that code that may handle too much think and seems useless for now.


celogeek commented Jan 7, 2013

That patch define the SessionFactory type.



ghost commented Jan 7, 2013

That illustrates my point. SessionFactory is the class name, but the config is under "session" under the "engines" config option. Should 'type' be "Session" or "SessionFactory"? Who knows.

Nothing that uses engines cares about the 'type' defined by Core::Role::Engine, so there's no point to making consumers of the class define one.


sukria commented Jan 22, 2013

I agree with @dagolden : if something is not used in the code, it should not be there.

@celogeek : do you agree we can remove that unused stuff?


sukria commented Jan 27, 2013

@celogeek : ping :)


yanick commented Feb 6, 2013

Fwiw: +1 also on the removal.


sukria commented Apr 21, 2013

Done. Thanks.

sukria pushed a commit that referenced this pull request Apr 21, 2013

@sukria sukria closed this Apr 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment