spatie / period

Complex period comparisons
https://spatie.be/open-source
MIT License
1.55k stars 72 forks source link

Add debug helper #7

Closed maidmaid closed 5 years ago

maidmaid commented 5 years ago

According to the first example of the doc :

$a = Period::make('2018-01-01', '2018-01-31');
$b = Period::make('2018-02-10', '2018-02-20');
$c = Period::make('2018-03-01', '2018-03-31');
$current = Period::make('2018-01-20', '2018-03-10');
$overlaps = $current->overlap($a, $b, $c);

Why not implementing __string() that would make a nice representation of the result like in your doc :

echo $overlaps; // ouput:

A       [========]
B                    [==]
C                            [=====]
CURRENT        [===============]
OVERLAP        [=]   [==]    [=]

It could be a very convenient way for debugging.

brendt commented 5 years ago

I also thought about that. It seems like a good idea, but it might be complex to get the formatting right. I also think this should be implemented as a dedicated method, and not using __toString. Do you want to give it a shot?

simensen commented 5 years ago

Would this be more of a debugging/utility method (so in another class) vs a proper method on the class itself?

brendt commented 5 years ago

@simensen that sounds like the correct approach. I wonder if this could be a simple static method, or if we should instantiate this debugger class.

thecrypticace commented 5 years ago

I'm going to work on this some today. My currently proposed API is this:

$output = $visualizer->visualize([
  "A" => $a,
  "B" => $b,
  "C" => $c,
  "CURRENT" => $current,
  "OVERLAPS" => $overlaps,
]);

I'm open to ideas here tho.

brendt commented 5 years ago

I believe a separate instantiated object is the way to go. Keep in mind that this should also be possible, but it should work with what you've proposed:

$output = $visualizer->visualize(...$collection);
maidmaid commented 5 years ago

Interesting. A suggestion a little bit different :

When we do

$overlaps = $current->overlap($a, $b, $c);

$overlaps should be aware it has been overlapped from $current, $a, $b and $c. Said differently :

$overlaps->parent === [
  'name' => 'current',
  'value' => $current,
  'method' => 'overlap',
  'args' => ['a' => $a, 'b' => $b, 'c' => $c]
] // true

Then, as $overlaps knows everything about how it has been made, we could imagine something easier for making a dump :

$output = $visualizer->visualize($overlaps);

It's done by passing simply $overlaps to the visualizer. Isn't it what we'd like to do when we'll debug it ?

thecrypticace commented 5 years ago

I thought about this idea but I'm not sure that's actually a good idea. Overlaps is just a collection of periods. You should be able to visualize any arbitrary collection. Perhaps if it was aware it should provide a convenience method but I'd be hesitant to add that.

Perhaps this could be considered as a followup feature once the visualization is done.

maidmaid commented 5 years ago

If I do :

echo $visualizer->visualize($current->overlap($a, $b, $c));

I expect :

A       [========]
B                    [==]
C                            [=====]
CURRENT        [===============]
OVERLAP        [=]   [==]    [=]

but not :

0        [=]
1              [==]
2                      [=]

No ?

thecrypticace commented 5 years ago

There's no way to get those variable names in PHP without super fancy reflection.

thecrypticace commented 5 years ago

And even then it might require parsing the code (maybe?)

maidmaid commented 5 years ago

Right... An easier way could be :

1       [========]
2                    [==]
3                            [=====]
parent         [===============]
overlap        [=]   [==]    [=]

1, 2, 3 are just the indexes of the arguments, parent references $current, and overlap show the result. Thus, no reflection.

thecrypticace commented 5 years ago

That only works if overlap retains a reference to parent which with the current API I don't believe is a good idea. If it returned a different object of type Overlap then yeah maybe.

I'll rattle around with the idea some. I want to get the actual bar visualization working first. Everything else is trivial otherwise.

brendt commented 5 years ago

Available as of 0.4.0

freekmurze commented 5 years ago

Very cool feature, thanks @thecrypticace 👍