Improve public API

[CyberShadow]

Attempts to fix #66.

[CyberShadow]

@doublep I think this should now include (some form of) solutions for all the items discussed in #66, could you have a look and see if this is enough for Eldev's Undercover integration?

[doublep]

It doesn't look like :files are actually activated. If I simply eval sth. like (undercover (:files "lol.el")), I get a complaint about unsupported option.

[CyberShadow]

Right, fixed.

[doublep]

report-on-kill is completely broken.

[CyberShadow]

How is it broken?

Are you trying to use it with an expression? I think I may need to tweak the macro for that to work...

[doublep]

Evaluated before being set. (unless undercover--report-on-kill should be (when ..., maybe something else. Just test yourself.

[CyberShadow]

Well spotted, thanks!

[doublep]

When disabling report-on-kill and calling undercover-safe-report manually, I have no idea if it has information or not (because e.g. it got disabled on a non-CI machine). So, I get an error message for nothing.

[CyberShadow]

Hmm, the semi-relying on defaults thing again.

I guess there's no harm in making undercover--coverage-enabled-p public, might as well. Could be useful for other scenarios such as working around bugs.

[doublep]

Hmm, the semi-relying on defaults thing again.

I don't want to duplicate the logic of "am I on CI or not?". It is easy to enable local report generation, but always doing it is annoyingly slow. Always disabling is also not a good idea as then you lose the "do the right thing without configuration on CI", which is 99% of usecases. So instead I use "default to CI only, unless explicitly told otherwise on command line" logic.

Please also make undercover--message public so that it can be advised. Alternatively, pipe all message through a user-provided function (see e.g. iter2-tracing-function here: it provides the default tracing, but can easily be replaced with something else too). Usecase: Eldev highlights errors/warning/normal output/debug output differently and sends to different channels, so I need to know the verbosity level associated with the message.

[CyberShadow]

Please also make undercover--message public so that it can be advised.

That's a really bad reason to make a function public.

[CyberShadow]

Putting undercover--message behind an indirect call means that in the future I can't make undercover--message a macro thatevaluates its arguments lazily depending on the log level, in case future versions will need to maybe log things that have a non-trivial cost to evaluate.

Should I keep the (when (<= level undercover--verbosity) check inside undercover--message? Implementers of undercover-message-function wouldn't be able to control whether the function is being called or not, aside from pre-emptively setting the verbosity level very high.

[CyberShadow]

Always disabling is also not a good idea as then you lose the "do the right thing without configuration on CI", which is 99% of usecases.

Yeah, that's not the problem and a good thing IMO. The tricky part is how to design everything to allow integrations such as Eldev, while not breaking the ability to improve what "do the right thing" means in future versions and new circumstances.

[doublep]

Sorry, I forgot about this PR.

Should I keep the (when (<= level undercover--verbosity) check inside undercover--message? Implementers of undercover-message-function wouldn't be able to control whether the function is being called or not, aside from pre-emptively setting the verbosity level very high.

Doesn't sound terribly important to me, but maybe you could abstract this away the same way as you have done with undercover-message-function. The default implementation would then use undercover--verbosity, but could be replaced. As of now, Eldev (the stash I have privately) does set verbosity to very high in order to filter later.

Yeah, that's not the problem and a good thing IMO.

Doesn't sound like a good thing to me. Essentially, "run by default only on a CI server" is a feature of undercover that I don't want to lose when it is run from Eldev. It is way too useful. If you want Eldev or whatever other higher-level wrapper to make this explicitly, expose undercover--under-ci-p publicly. But as it stands, it does not look really necessary, as undercover-enabled-p is now public.