Add sprintf wrappers

[guillomovitch]

Summary

Add sprintf-based wrappers over standard logging methods (debug(), info(), error(), ...)

Motivation

This is just a port of a Log::Any feature:
https://metacpan.org/pod/Log::Any#Logging

[kathackeray]

There is a fair amount of duplication between _logf() and the original _log(). Might it be preferable to perform only the sprintf in _logf() and then hand off to _log() to do what it does uniquely?

As for the concept of adding new functions for this purpose rather than using sprintf, it will be better for others to comment.

[guillomovitch]

Agreed, here is a new proposal.

[kraih]

What are the use cases for this feature?

[rmallah]

This feature is very useful.
Although i use https://metacpan.org/pod/Mojolicious::Plugin::Logf
for the same. Embedded %s are useful for not having to use
concat while logging multiple variables in the same line.

regds
Rajesh.

[kraih]

You are aware that string interpolation exists in Perl, right?

[guillomovitch]

Sure, and sprintf() exists too. Its main advantage is to allow easier code formatting across multiple lines for long strings with multiple variables, avoiding use of intermediate variables for function call.

[kraih]

"previous method" is easy to break when new methods are added.

[kraih]

No tests.

[kathackeray]

Personally, I question whether the occasional and fairly minor convenience is worth doubling up the log methods and enveloping language features, when $log->error( sprintf('%s is %s', 'Mojolicious', 'cool') ) does the job when needed. But it does seem to have support. Maybe this is more common than I am aware.

[rmallah]

You are aware that string interpolation exists in Perl, right?

Surely :-)

$c->logf( debug => "age of student %s is : %s" , $obj->name , $obj->age );

interpolation still does not (imho should not) handle all expression evaluations.

regds
Rajesh.

[rmallah]

Personally, I question whether the occasional and fairly minor convenience is worth doubling up the log methods and enveloping language features, when $log->error( sprintf('%s is %s', 'Mojolicious', 'cool') ) does the job when needed. But it does seem to have support. Maybe this is more common than I am aware.

its already difficult to push lazy programmers to do requisite amount of logging in
code. making 'em write an extra sprintf makes it even more challenging.

moreover such features are found even in other logging contexts/environment.
Eg:
https://www.postgresql.org/docs/9.3/plpgsql-errors-and-messages.html

Pull request has been modified.

[kathackeray]

Judging from the feedback on other reviews, you'll be asked to squash your commits.

[kathackeray]

The convention seems to be capitalisation for subtest names, e.g. subtest 'History' => sub {, unless the subtest name is a function name, in which case it is quoted, e.g. subtest '"error"' => sub {.

As this tests specifically your new sprintf wrappers, in my opinion it should be something like Text formatting wrappers for the subtest name.

[guillomovitch]

I just pushed a new version, with commit squashed as requested

[jberger]

I'm with some of the other voices here, this functionality is easy to access by other mechanisms and thus does not need to be implemented by core mojo. It can also be provided by a role on CPAN. However it would increase the complexity of the module, adding nearly double the number of methods.

I know some other logging frameworks use this style of logging by default, and so I will not vote against if enough people think that is worthwhile, but I will also not vote in support.

[tianon]

Given every one of them is just sub foof { shift->foo(sprintf(shift, @_)) }, couldn't they be defined in a code loop somehow instead? I seem to recall other modules in Mojo doing something like that.

I'm not a maintainer, but I agree that a CPAN module providing these seems to make more sense though. 😇

[jhthorsen]

I do like my logf() plugin (though it should be a Mojo::Log role these days), and from the experience of writing multiple implementations, I don't think a plain "sprintf" is enough in many cases.

Based on that I'm not in favor of adding this to core.

[daleif]

I find this discussion very interesting.

@jhthorsen care to elaborate on I don't think a plain "sprintf" is enough in many cases.?

[jhthorsen]

@jhthorsen care to elaborate on I don't think a plain "sprintf" is enough in many cases.?

Once I need sprintf, I also often need some sort of Data::Dumper or encode_json to make it useful. Other than that I think the most useful feature is handling undef in some sensible way. The only thing this PR adds (imo) is that you don't have to do "foo=@{[$obj->foo]}", which I don't find that useful.

[kathackeray]

In terms of the code alone, the tests pass and documentation is adequate. I believe a change is required to the test messages, see annotation below.

However, I still feel that this concept isn't in the minimal spirit of core Mojo.

I appreciate the 'lazy developers' argument mentioned above, and there are occasions it might have sway. But that argument is unavoidably tied to what the developers are being asked to do, e.g. if another set of lazy developers are being asked to log entirely in uppercase, but they're too lazy to uc themselves, should core Mojo have to accommodate with a set of erroruc(), warnuc() etc. methods? I think rather it is the duty of an extension.

Additionally the existing logf plugin is fuller featured than the implementation here. Striving to handle undefined values and complex data structures (surely a boon for lazy developers). If the functionality was to be integrated into core, I might expect it to at least live up to that standard.

[kathackeray]

Test messages are not differentiated for the log level.

[rmallah]

IMHO , keeping the spirit of minimalism the function may be left and improved further in plugin Logf as it is
already more complete and useful implementation rather than plain sprintf.

Can we not have a curated list of very useful Mojo plugins and have that list suggested in the documentation?

In terms of the code alone, the tests pass and documentation is adequate. I believe a change is required to the test messages, see annotation below.

However, I still feel that this concept isn't in the minimal spirit of core Mojo.

I appreciate the 'lazy developers' argument mentioned above, and there are occasions it might have sway. But that argument is unavoidably tied to what the developers are being asked to do, e.g. if another set of lazy developers are being asked to log entirely in uppercase, but they're too lazy to uc themselves, should core Mojo have to accommodate with a set of erroruc(), warnuc() etc. methods? I think rather it is the duty of an extension.

Additionally the existing logf plugin is fuller featured than the implementation here. Striving to handle undefined values and complex data structures (surely a boon for lazy developers). If the functionality was to be integrated into core, I might expect it to at least live up to that standard.

[guillomovitch]

I didn't expected so much discussion for such a feature, which is indeed trivial to implement outside of Mojo. I prefer to close the integration request rather than continue to try to satisfy minor cosmetics issues, while the general consensus seems to reject the change.