async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
170 stars 9 forks source link

Loop::info #102

Closed kelunik closed 7 years ago

kelunik commented 8 years ago

This method currently doesn't match the other naming schemes. Should we rename it to getInfo? Any other name suggestions?

bwoebi commented 8 years ago

I see info() as an abbreviation of inform() … I do not see how getInfo() is more expressive/better conveying intent here.

kelunik commented 7 years ago

Maybe we should find a entirely new name like getWatcherInfo or getWatcherStats, because that's what it actually returns.

bwoebi commented 7 years ago

The minimum info is only about watchers, yes. But implementations may want to add more loop specific info.

Apart from that getWatcher*() makes me expect the first parameter to be a watcher Id and get stats/info on that specific watcher.

kelunik commented 7 years ago

Maybe we should drop that minimum, as anything other isn't guaranteed to be there anyway. If you have to check for a specific implementation anyway if you want to use that data, you can call another specific method, too.

bwoebi commented 7 years ago

I see no explicit advantage of dropping that minimum.

kelunik commented 7 years ago

It doesn't encourage adding "unrelated" entries.

bwoebi commented 7 years ago

And that's an issue because…?

kelunik commented 7 years ago

Depends what you do with the data. If you use code like the following for submitting metrics, the code might break if other values are provided with a different structure.

foreach (Loop::info() as $type => $values) {
    foreach ($values as $prop => $count) {
        $metrics->count("{$type}.{$prop}", $count);
    }
}
kelunik commented 7 years ago

@trowski @joelwurtz What are your opinions on that? You put a reaction on the OP, but didn't comment here.

trowski commented 7 years ago

@kelunik I'd rather see the name be getInfo() purely for consistency. It might be nice to add __debugInfo() to the interface as an alias to this method.

bwoebi commented 7 years ago

@trowski I wouldn't recommend adding __debugInfo() to the interface (as in you shouldn't make magic funcs part of the interface). We may recommend it, but I wouldn't force it.

trowski commented 7 years ago

Ok, that's reasonable, was just a random thought. :smile:

kelunik commented 7 years ago

@bwoebi You're the only one against, still have strong feelings against getInfo?

@trowski @WyriHaximus What do you think about removing the minimum and requiring those exact values?

bwoebi commented 7 years ago

Well, what shall I say… it's just a name. Names are subjective by definition. If I'm outnumbered here, then go with it.

I have stronger feelings about the minimum. I still see no issue with having it. It just is an artificial unnecessary limit.

kelunik commented 7 years ago

@bwoebi As I said in https://github.com/async-interop/event-loop/issues/102#issuecomment-258705894, it depends on how you consume those values.

bwoebi commented 7 years ago

That's why you should be aware of that and code appropriately.

WyriHaximus commented 7 years ago

@kelunik honestly I like the minimum as it gives flexibility for drivers to expose more values, and feel rather strongly about it. What we could do is require any additional values to be structured the same way as the minimum, for example:

'foo' => [
    'bar' => 'bier',
],