Add default logger

[jcupitt]

add a thing to make a simple debug logger to Image ... this makes it a one-liner to enable debug output

Perhaps this should be a separate class? It seems a bit stange to make an anon class in a member function like this, and phpcs hates it. @kleisauke, do you know what php typically does for this kind of thing?

[koenpunt]

Adding an actual Logger class is probably more common.

[jcupitt]

OK, moved to Main and Logger. Does that look OK?

Issue for this PR #17

[koenpunt]

Logger looks fine. But I now see that Vips\Main class for the first time, so while talking about "common", I've never seen a class named Main in any library that I used. And I also think it can be confusing in an application because it can refer to anything. IMO it should be Vips\Vips instead of Vips\Main. Just my 2 cents.

Nb. Great work you're doing on this, even though I think you're not even using the library yourself! 💯

[jcupitt]

Vips\Vips sounds a bit odd to me. How about Vips\Common, would that be more normal? I'll add a note to the 1.0 TODO to think about the name of Main again.

No, I won't be using it myself, it's just an experiment. I'm thinking of moving the Python binding (which I do use) to this model, so it's a try-out.

[koenpunt]

What about Vips\Config? If we (re)move the debug and error methods I think that fits perfectly.

Not sure about convention in PSR, but can't debug and error be just functions under the namespace?

E.g.:

namespace Vips; /**  * Log a debug message.  *  * @param string $name The method creating the messages.  * @param array $arguments The method arguments.  *  * @return void  */ function debug(string $name, array $arguments): void { $logger = Config::getLogger(); if ($logger) { $logger->debug($name, $arguments); } } /**  * Log an error message.  *  * @param string $message The error message.  * @param \Exception $exception The exception.  *  * @return void  */ function error(string $message, \Exception $exception): void { $logger = Config::getLogger(); if ($logger) { $logger->error($message, ['exception' => $exception]); } }

[jcupitt]

I think everything has to be inside a class for autoload to work, hence the apparently useless Main singleton.

[koenpunt]

I guess you're right, than maybe wrap them [debug and error method] in a Vips\Utils class?

[jcupitt]

OK, I've added that as another idea on #14

[kleisauke]

We could remove these unused lines, because they are defined in Vips\Main now.

[jcupitt]

Ooop! Drat, I was sure I'd done that, thanks.

[kleisauke]

Nitpick: drop the first \:
use Psr\Log\LoggerInterface;

[kleisauke]

We could import also the LoggerTrait here, like: use Psr\Log\LoggerTrait;

[kleisauke]

If we imported the LoggerTrait, then we can just use: use LoggerTrait;.

[kleisauke]

Maybe rename to DebugLogger.

[kleisauke]

Nitpick: drop the first \:
use Psr\Log\LoggerInterface;

[kleisauke]

We could use: Main::debug($name, ['instance' => $instance, 'arguments' => $arguments)]); instead of the array_merge.

[kleisauke]

Some general comments added. I think it's fine to move the logic of the logger to a separate class. Not sure how we should name this class (perhaps Vips\Main -> Vips\Config, like this example?).

Instead of Vips\Logger I think we should rename it to Vips\DebugLogger so that users know what the class is doing on it's name.

Overall, I like the idea of making a one-liner to enable debug output.

[jcupitt]

OK, all comments covered, I think.

[koenpunt]

@kleisauke I'm pro-Vips\Config, but the logError and logDebug function have nothing to do with configuration. That's why I proposed an additional Vips\Utils class. But [I believe] discussion has been moved to #14

[jcupitt]

OK, left it as Config for now.