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

Router: Have a routes collection interface & support in Tracy router panel #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Router: Have a routes collection interface & support in Tracy router panel #94

wants to merge 4 commits into from

Conversation

Andrewsville
Copy link

  • Feature
  • Documentation – No change needed, unless you want to mention it in the "Custom Router" chapter.
  • BC break - no

Currently, the only way to have a collection of routes and have them displayed in the Tracy panel is to use the RouteList class.

This PR generalizes this logic to allow developers to implement their own route collections maybe with some logic inside (the RouteList is just a simple hashmap), maybe immutable (RouteList is mutable), etc. It comes from our use case but I believe it could be useful for others, too.

  • This PR introduces a new interface for route lists that extends IteratorAggregate (which is the easiest way to implement a Traversable) and adds the getModule() method that is required by the Tracy panel.
  • Because of the way RouteList is implemented, it can implement this new interface without any changes.
  • And the last change is the instanceof typecheck in the Panel to display a routes collection.

@pavelkouril
Copy link
Contributor

Personally, I like this idea. :) 👍

/**
* Routes collection.
*/
interface IRouteList extends \IteratorAggregate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not extend Traversable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not extend IRouter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User-land code can't extend Traversable directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface can extend, but class can't implement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is funny:

this works:

interface A extends Traversable 
{}

class B implements IteratorAggregate, A
{
    function getIterator() {}
}

this triggers Fatal error: Class B must implement interface Traversable as part of either Iterator or IteratorAggregate:

interface A extends Traversable 
{}

class B implements A, IteratorAggregate
{
    function getIterator() {}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have encountered this (the need to have interfaces in the correct order) when developing the PR and it is exactly the reason I won't implement it using Traversable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JanTvrdik
Copy link
Contributor

I dislike the getModule() method onIRouteList.

@Andrewsville
Copy link
Author

Well, it is required by the Panel. There's not much I can do about it. You could say the same about the RouteList itself where its sole purpose is to be printed by the panel.

@dg dg force-pushed the master branch 11 times, most recently from 8eb9618 to 5d63a8d Compare May 5, 2016 19:22
@dg dg force-pushed the master branch 5 times, most recently from c7531dc to b7a311f Compare May 13, 2016 13:11
@dg dg force-pushed the master branch 5 times, most recently from 12d577b to e04c0e1 Compare May 20, 2016 11:42
@dg dg force-pushed the master branch 2 times, most recently from 7b1ec30 to 3adfa43 Compare May 23, 2016 01:09
@dg dg force-pushed the master branch 4 times, most recently from c73b255 to 9e7cd60 Compare June 7, 2016 10:30
@dg dg force-pushed the master branch 2 times, most recently from ea11e9d to 8ff194f Compare June 16, 2016 14:57
@f3l1x
Copy link
Member

f3l1x commented Jun 21, 2017

I like it. Could we merge it, rebase it or close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants