introduce a lazy iterating route collection #101

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@dawehner
Contributor

dawehner commented Mar 31, 2014

Note: This solves #97 potentially, though you cannot implement Iterator and IteratorAggregate at the class

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Mar 31, 2014

Member

thx. fyi your changes to LazyRouteCollection are not PSR-2 compliant

Member

lsmith77 commented Mar 31, 2014

thx. fyi your changes to LazyRouteCollection are not PSR-2 compliant

+ *
+ * @return \ArrayIterator An \ArrayIterator object for iterating over routes
+ */
+ public function getIterator()

This comment has been minimized.

@dbu

dbu Apr 1, 2014

Member

is this for BC with the pre-alpha code or is it a requirement by the routing? and as we are an iterator already, can't we simply return $this?

@dbu

dbu Apr 1, 2014

Member

is this for BC with the pre-alpha code or is it a requirement by the routing? and as we are an iterator already, can't we simply return $this?

This comment has been minimized.

@dawehner

dawehner Apr 8, 2014

Contributor

Well ... a class can't implement iterator and iteratorAggregate at the same time so we are pretty much fucked up, given that RouteCollection is the thing people people typehint. One way would be to override getIterator and prodive a total different class for it.

@dawehner

dawehner Apr 8, 2014

Contributor

Well ... a class can't implement iterator and iteratorAggregate at the same time so we are pretty much fucked up, given that RouteCollection is the thing people people typehint. One way would be to override getIterator and prodive a total different class for it.

This comment has been minimized.

@dbu

dbu Apr 15, 2014

Member

oh you are right. then i guess we just keep it this way. its a bit stupid but adding more classes will not make things more efficient.

@dbu

dbu Apr 15, 2014

Member

oh you are right. then i guess we just keep it this way. its a bit stupid but adding more classes will not make things more efficient.

+ /**
+ * Contains the amount of route which are loaded on each provider request.
+ */
+ const ROUTE_LOADED_PER_TIME = 50;

This comment has been minimized.

@dbu

dbu Apr 1, 2014

Member

could we make this a constructor argument with a default value instead, so it can be made configurable?

@dbu

dbu Apr 1, 2014

Member

could we make this a constructor argument with a default value instead, so it can be made configurable?

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Apr 1, 2014

Member

thanks a lot @dawehner , very cool!

the testIteratingSmall test is failing on travis, can you check that please?

Member

dbu commented Apr 1, 2014

thanks a lot @dawehner , very cool!

the testIteratingSmall test is failing on travis, can you check that please?

+ * @return \Symfony\Component\Routing\Route[]
+ * Routes keyed by the route name.
+ */
+ public function getRoutesRanged($offset, $length = NULL);

This comment has been minimized.

@dbu

dbu Apr 1, 2014

Member

i think we should add a getRoutesCount method as well, to replace the count method. that currently still will load all routes.

@dbu

dbu Apr 1, 2014

Member

i think we should add a getRoutesCount method as well, to replace the count method. that currently still will load all routes.

+ /**
+ * {@inheritdoc}
+ */
+ public function current() {

This comment has been minimized.

@dbu

dbu Apr 1, 2014

Member

{ goes on next line, also below

@dbu

dbu Apr 1, 2014

Member

{ goes on next line, also below

This comment has been minimized.

@dawehner

dawehner Apr 8, 2014

Contributor

I still need to figure out how to configure storm to use different codestyles for different projects.

@dawehner

dawehner Apr 8, 2014

Contributor

I still need to figure out how to configure storm to use different codestyles for different projects.

+ */
+ protected $currentRoute = 0;
+
+ public function __construct(RangedRouteProviderInterface $provider)

This comment has been minimized.

@dbu

dbu Apr 1, 2014

Member

like this, this is a BC break. could we instead extend LazyRoutesCollection as a RangedRoutesCollection that adds the features?

btw, just a question: why the name "Ranged"? why not PagingRouteProvider and PagedRouteCollection?

@dbu

dbu Apr 1, 2014

Member

like this, this is a BC break. could we instead extend LazyRoutesCollection as a RangedRoutesCollection that adds the features?

btw, just a question: why the name "Ranged"? why not PagingRouteProvider and PagedRouteCollection?

This comment has been minimized.

@dawehner

dawehner Apr 8, 2014

Contributor

Oh sure, I just did not had a better idea for a name. Thank you

@dawehner

dawehner Apr 8, 2014

Contributor

Oh sure, I just did not had a better idea for a name. Thank you

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Apr 1, 2014

Member

okay, looked over this. what do you think @dawehner ?

Member

dbu commented Apr 1, 2014

okay, looked over this. what do you think @dawehner ?

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Apr 4, 2014

Member

ping @dawehner

Member

dbu commented Apr 4, 2014

ping @dawehner

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Apr 8, 2014

Contributor

Sorry for being unresponsive.

Contributor

dawehner commented Apr 8, 2014

Sorry for being unresponsive.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Apr 15, 2014

Member

now i was unresponsive because i was absent. back now, would love to wrap this up.

Member

dbu commented Apr 15, 2014

now i was unresponsive because i was absent. back now, would love to wrap this up.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu May 5, 2014

Member

ping

Member

dbu commented May 5, 2014

ping

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Jun 12, 2014

Member

ping

Member

dbu commented Jun 12, 2014

ping

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Jun 12, 2014

Contributor

Currently involved in my thesis, so please don't expect at least something
from me here in the next time

On Thu, Jun 12, 2014 at 8:31 AM, David Buchmann notifications@github.com
wrote:

ping


Reply to this email directly or view it on GitHub
#101 (comment).

Contributor

dawehner commented Jun 12, 2014

Currently involved in my thesis, so please don't expect at least something
from me here in the next time

On Thu, Jun 12, 2014 at 8:31 AM, David Buchmann notifications@github.com
wrote:

ping


Reply to this email directly or view it on GitHub
#101 (comment).

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Jun 12, 2014

Member

ah okay. will see if i find time to wrap things up. good luck with the thesis then!

Member

dbu commented Jun 12, 2014

ah okay. will see if i find time to wrap things up. good luck with the thesis then!

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Jun 12, 2014

Contributor

thank you for both

On Thu, Jun 12, 2014 at 10:04 AM, David Buchmann notifications@github.com
wrote:

ah okay. will see if i find time to wrap things up. good luck with the
thesis then!


Reply to this email directly or view it on GitHub
#101 (comment).

Contributor

dawehner commented Jun 12, 2014

thank you for both

On Thu, Jun 12, 2014 at 10:04 AM, David Buchmann notifications@github.com
wrote:

ah okay. will see if i find time to wrap things up. good luck with the
thesis then!


Reply to this email directly or view it on GitHub
#101 (comment).

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 Aug 7, 2014

Member

ping

Member

lsmith77 commented Aug 7, 2014

ping

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Aug 10, 2014

Contributor

meh, yeah I cannot promise something but there is a chance on the trip now

Contributor

dawehner commented Aug 10, 2014

meh, yeah I cannot promise something but there is a chance on the trip now

@dawehner

This comment has been minimized.

Show comment
Hide comment
@dawehner

dawehner Aug 20, 2014

Contributor

This one can be closed in favour of the other one.

Contributor

dawehner commented Aug 20, 2014

This one can be closed in favour of the other one.

@dawehner dawehner closed this Aug 20, 2014

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