daylerees / anbu

Anbu profiler for the Laravel PHP Framework.
308 stars 24 forks source link

Data serialization #23

Closed garygreen closed 10 years ago

garygreen commented 10 years ago

Is there any reason why the when data is serialized it's done in a real basic index-like fashion? E.g. for the routing table:

        // Add a new route to the data array.
        $this->data['routes'][] = [
            $route->getMethods(),                               // HTTP Verb
            $this->highlightParams($route->getPath()),          // URI
            $route->getActionName()                             // Action
        ];
    }

Then when outputing in the view:

    <tr>
        <td><span class="method-tag method-tag-{{ strtolower($current[0]) }}">{{ $current[0] }}</span></td>
        <td class="code">{{ $current[1] }}</td>
        <td class="code">{{ $current[2] }}</td>
    </tr>

Would it be better to have named keys?

    <tr>
        <td><span class="method-tag method-tag-{{ strtolower($current['verb']) }}">{{ $current['verb] }}</span></td>
        <td class="code">{{ $current['uri'] }}</td>
        <td class="code">{{ $current['action'] }}</td>
    </tr>
garygreen commented 10 years ago

Reason why I've run into this is because I forked the repo to add a column in the table for the route name (was going to do a PR) but would like it to appear after the URI (and keep the controller/action always in the far column). If I added it, it would break outputting data in the existing requests. So in it's current form, either need to a. add it to the end or b. change the way data is serialized so it's keyed (which will likely mean breaking existing request data stored). I think relying on the index order to determine what data it is possibly isn't an ideal format? What are your thoughts @daylerees ? :smile:

daylerees commented 10 years ago

They can be changed to keys, I just didn't need it at the time. :)

daylerees commented 10 years ago

Of course a downside would be that people updating to this change, would have to clear their history, but hey, it's Alpha for a reason!

garygreen commented 10 years ago

Yeah exactly. The change will help for future updates though. I'll do a PR. I don't know about you but I personally don't anticipate to keep my requests for any longer than a day (would be cool if there was some kind of automatic clean/purge functionality, clearing any data older than X days as per config setting. I can do a PR for that as well if you like?). ;-)

daylerees commented 10 years ago

Awesome on the first PR, leave the second one with me please. I’m working on new adapters for the storage mechanism, and it would be nice to have them consistent. :)

Dayle Rees Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Friday, 19 September 2014 at 17:16, Gary Green wrote:

Yeah exactly. The change will help for future updates though. I'll do a PR. I don't know about you but I personally don't anticipate to keep my requests for any longer than a day (would be cool if there was some kind of automatic clean/purge functionality, clearing any data older than X days as per config setting. I can do a PR for that as well if you like?). ;-)

— Reply to this email directly or view it on GitHub (https://github.com/daylerees/anbu/issues/23#issuecomment-56199315).

garygreen commented 10 years ago

Closing this as added PR #24