Improve public API

[doublep]

Currently, undercover is very difficult to use from other tools, as it provides very few public functions, largely tailored to "the standard way of running".

Based on Eldev integration code, which currently often has to dive into internals, I can suggest/request the following improvements:

• Don't require to go through undercover macro. Currently it largely looks like

  (defun im-the-only-entry-point () (step-1) (step-2) (step-3))

  instead, make all the step-# functions public and let users just call them in the correct order, easily preprocessing arguments or postprocessing results, calling whatever inbetween and so on.

• Instead or in addition to passing configuration through undercover--set-options, make the variables it sets, like undercover--report-file-path, public.
• Allow callers to specify file lists, don't insist on processing wildcards in it.
• Don't call undercover-report-on-kill automatically. Instead, let the caller to generate report when he wants.
• Expose the default value of undercover--report-file-path somehow. Since 0.8 it is practically impossible to dig it out of the library. Maybe just add a function that returns it.
• Clean up, document and expose a way to merge reports vs. saving new. Currently, the only way to start from scratch seems to be manually deleting report file, which is further complicated by inability to find the name (previous point). Starting from scratch seems to be "the sane" option for local reports, while about merging I'm not even sure when it is useful.

Naturally, there can and should be way to work "as now", probably implemented as simple helpers over more common API outlined above.

[CyberShadow]

Thank you for filing this. It's a good start.

• Don't require to go through undercover macro. Currently it largely looks like

  (defun im-the-only-entry-point ()
  (step-1)
  (step-2)
  (step-3))

  instead, make all the step-# functions public and let users just call them in the correct order, easily preprocessing arguments or postprocessing results, calling whatever inbetween and so on.

• Instead or in addition to passing configuration through undercover--set-options, make the variables it sets, like undercover--report-file-path, public.

It would be more helpful if you described this in terms of what the library needs to achieve, as opposed to what the API may look like to satisfy those requirements.

I don't see making the internal variables public as a good change - it would be confusing at best. The undercover macro could instead have a mode where it does nothing but set those variables.

* Allow callers to specify file lists, don't insist on processing wildcards in it.

Can be done by adding (:files "a.el" "b.el") as a recognized CONFIGURATION item.

* Don't call `undercover-report-on-kill` automatically. Instead, let the caller to generate report when he wants.

Can be done by adding a new CONFIGURATION item.

The function to generate a report is already public.

• Expose the default value of undercover--report-file-path somehow. Since 0.8 it is practically impossible to dig it out of the library. Maybe just add a function that returns it.

• Clean up, document and expose a way to merge reports vs. saving new. Currently, the only way to start from scratch seems to be manually deleting report file, which is further complicated by inability to find the name (previous point). Starting from scratch seems to be "the sane" option for local reports, while about merging I'm not even sure when it is useful.

Wouldn't it be better and easier to set the report path explicitly? This way, the tool using Undercover has control over the "lifetime" of the report file, and is thus in control whether merging happens or not. It should also be in control of the report format, as e.g. merging text reports is not meaningful/possible.

Tools that attempt to provide a stable API on top of Undercover should not depend on Undercover defaults, as they are subject to change. This includes the default value of undercover--report-file-path, as its meaning depends on :report-format, which is auto-detected by default (and thus its implicit value may change in future Undercover versions). Instead, tools should attempt to explicitly configure as many of Undercover's settings as applicable.

[doublep]

It would be more helpful if you described this in terms of what the library needs to achieve, as opposed to what the API may look like to satisfy those requirements.

I don't see making the internal variables public as a good change - it would be confusing at best. The undercover macro could instead have a mode where it does nothing but set those variables.

Eldev provides a somewhat different interface to the same functionality. You have an intermediate representation called configuration. It is heavily tailored to how undercover.el is configured from Cask file, but for other uses may be nothing more than a hassle.

Basically, you extract key-value pairs from configuration and set variables identified by keys to the provided values, with a few tweaks. That's pretty much all that undercover--set-options does. So, from my point of view it would be easier both for undercover.el and its users (Eldev in this case) if you just allowed to set the variables directly. As it stands, Eldev has to construct this configuration list with the sole of passing it to undercover--set-options, which promptly deconstructs it. Eldev could just assign to variables directly instead.

Can be done by adding (:files "a.el" "b.el") as a recognized CONFIGURATION item.

And undercover wouldn't need to add more supported items to configuration every time if it just exposed some variables. In this case it probably would not be a variable, even, simply an argument to exposed undercover--edebug-files function.

Can be done by adding a new CONFIGURATION item.

Or can also be achieved by not following framework pattern (there are 5 functions I will call, tell me which you don't need, or how I should tweak their arguments for you), but instead library pattern (here are 5 functions, call them as you like; I also have one convenience function that calls them all, in case you don't need fine control). If you just expose the step functions, I could simply avoid calling the one that modifies kill-emacs-hook. I could call eldev-trace or something else between the steps if I needed.

Instead, tools should attempt to explicitly configure as many of Undercover's settings as applicable.

Well, in this case I'd like to pass the default onto user. Eldev is meant to be more interactive than e.g. Cask, it is suitable for running from command line, generate local reports and so on. I would like to achieve such things as "if you don't provide report filename, it will be generated in file XYZ", where XYZ comes from undercover.el somehow. Especially important for things like SimpleCov, where filename is important, but I'd rather have undercover.el control it, than Eldev.

[CyberShadow]

It is heavily tailored to how undercover.el is configured from Cask file,

What do you mean by this? As far as I know, Cask doesn't provide a way to specify and pass configuration on to Undercover from the Cask file.

As it stands, Eldev has to construct this configuration list with the sole of passing it to undercover--set-options, which promptly deconstructs it. Eldev could just assign to variables directly instead.

I think that's perfectly fine. It is much more valuable to retain the ability to modify the Undercover internals while providing a minimal stable public API, rather than cement ourselves into a particular way of doing things after making all our internal guts public. Some of these global variables may even be entirely removed in a future Undercover version.

And undercover wouldn't need to add more supported items to configuration every time if it just exposed some variables.

The options are the public interface. The variables are implementation details. I think it's important to hold on to that distinction. The two may represent the same data identically, or completely differently. For example, what if future versions of Undercover were to add the ability to add exclusions at the function level?

simply an argument to exposed undercover--edebug-files function

As far as users of Undercover are concerned, the fact that Undercover uses edebug to achieve its goal should be an implementation detail :)

Or can also be achieved by not following framework pattern (there are 5 functions I will call, tell me which you don't need, or how I should tweak their arguments for you), but instead library pattern (here are 5 functions, call them as you like; I also have one convenience function that calls them all, in case you don't need fine control). If you just expose the step functions, I could simply avoid calling the one that modifies kill-emacs-hook. I could call eldev-trace or something else between the steps if I needed.

I understand the more important distinction between the two approaches here is "do what I say" vs "this is what I want to do".

Two downsides of your proposal:

• If, during the course of development of a project using the library, one realizes that they need to switch from the one convenience function to the many low-level functions, then this involves a sudden large code change (larger than configuring the convenience function).
• The contents of the convenience function is subject to change in future versions without affecting the public API. All users of the library which mimic it using the low-level API may need to perform similar changes on their end.

Since we're talking about something as simple as disabling automatic report generation, simply making this configurable makes much more sense to me.

I would like to achieve such things as "if you don't provide report filename, it will be generated in file XYZ", where XYZ comes from undercover.el somehow.

Generally, this is not possible, and doesn't really make sense outside of specific situations. The default report file name depends on the report type, which is autodetected depending on the environment that Undercover runs in. Furthermore, the text report type can produce its output by message (so no file is involved), and some coverage file formats consist of multiple files instead of one (though none required by any coverage service supported by Undercover so far). This doesn't pose a problem to Undercover's API for configuring it, as it could add new options or make invalid some combinations of existing options appropriately, but it would be a problem for an API for querying Undercover's decisions.

If the goal is to allow the user to configure Undercover directly (not through Eldev) or let Undercover decide what to do based on its defaults, then my suggestion would be to reconsider and minimize Eldev's assumptions about what Undercover does. Alternatively, Eldev could provide its own interface for configuring Undercover, in which case Eldev would be in complete control over everything.

Perhaps we should take a step back and look at a list of high-level goals that Eldev is trying to achieve with regards to its Undercover integration?

[doublep]

Perhaps we should take a step back and look at a list of high-level goals that Eldev is trying to achieve with regards to its Undercover integration?

• Option not to modify kill-emacs-hook, but let Eldev call undercover-[safe-]report on its own.
• Generate report on given .el files, without wildcards or whatever. Just a list of strings.

• Eldev prints a lot of debugging information about what it is doing. In particular, the integration plugin contains these lines:

  (eldev-advised ('undercover--edebug-files :before (lambda (files &rest _ignored) (eldev-verbose "Instrumenting %s for collecting coverage information with `undercover'" (eldev-message-plural (length files) "file"))))

  So, ideally there should be a way to execute arbitrary code when undercover.el prepares to do something with .el files — be that edebug-instrumentation or whatever. I'm not printing this before even calling undercover.el because the library can decide not to do anything, e.g. if not on a CI server. With the framework approach the only sane ways I can see would be a hook or yet another configuration setting accepting a lambda. Or ignoring this.

• There should be a way to start reports from scratch instead of merging. Especially if, as you say, reports can span multiple files: would be much easier if undercover.el could delete (or ignore and just overwrite) everything than if the caller had to do it manually. Of course, Eldev prints another message here, so yet another hook, lambda configuration setting or ignore.

• Ideally I would like to get rid of this:

  ;; Since undercover' is a macro, we have to do it like this. (eval(undercover ,@files ,@(cdr configuration)) t)

  i.e. I'd prefer a function instead of/in addition to the macro.

[CyberShadow]

* Option not to modify `kill-emacs-hook`, but let Eldev call `undercover-[safe-]report` on its own.

OK, added (:report-on-kill nil), please see #67

* Generate report on given `.el` files, without wildcards or whatever. Just a list of strings.

OK, added (:files ...)

* Eldev prints a lot of debugging information about what it is doing. In particular, the integration plugin contains these lines:

Hmm... in absence of a unified logging framework in Emacs Lisp, how about the following:

1. Undercover consistently uses the string prefix "UNDERCOVER: " for the messages/errors it prints [done]
2. Undercover acquires a (:verbosity NUMBER) option, which Eldev could set to its maximum setting
3. Eldev advises message, and redirects Undercover's messages to its own logging printer.

* There should be a way to start reports from scratch instead of merging.

Added (:merge-report nil), does that work?

* Of course, Eldev prints another message here, so yet another hook, lambda configuration setting or ignore.

What message does this refer to?

* Ideally I would like to get rid of this:

Hmm... I understand this to be a common issue with programmatic use of macros. I don't see any notable downsides in your existing solution, other than the small hit to readability.

[doublep]

Hmm... in absence of a unified logging framework in Emacs Lisp, how about the following:

Yeah, sort of would work, but you are overcomplicating things and make it difficult for everyone. What if another user wanted to do something else other than logging at that point? Could have just made sth. like (undercover-prepare-for-files file-list) public (if you want to keep edebug as internals) to solve both this point (let users just log/do whatever before calling) and the point about file list. But I guess we already discussed this.

What message does this refer to?

Current code:

(if (plist-get (car configuration) :merge)
    (eldev-trace "Code coverage report will be merged with existing")
  (if effective-report-name
      (when (file-exists-p effective-report-name)
        (delete-file effective-report-name)
        (eldev-trace "Deleted previous code coverage report; new one will be restarted from scratch"))
    (unless (eq report-format 'text)
      (eldev-warn "Cannot determine where coverage report is generated; unable to honor `restart' flag"))))

Basically, a message that says if the previous report is deleted or merged with. Or no message if there is no previous report.

[CyberShadow]

Yeah, sort of would work, but you are overcomplicating things and make it difficult for everyone. What if another user wanted to do something else other than logging at that point?

Like what?

I think we need to determine who exactly this "everyone" is and what are their use cases before we can progress on this. We certainly can't design an API around conjecture or just add a myriad hooks at every possible point that someone may want to do "something".

[doublep]

Like what?

I have no idea. Abort after the first step without actually instrumenting to check if configuration is correct? Something else? In any case, I think that when writing something reusable, it is best not to assume how it might be used and not restrict potential usecases unless there are really compelling reasons.

We certainly can't design an API around conjecture or just add a myriad hooks at every possible point that someone may want to do "something".

Of course. A possible solution here would be splitting the undercover call into several public functions, then hooks in any form would not be necessary: user of the API would just have full control if he wanted it and wouldn't need to hook to anything. But I'm repeating myself.

I think we have presented our arguments already. Proceed as you decide, this is your project. Eldev, as the higher level here, will have to adapt.

[CyberShadow]

Yeah, I really don't see a way of making it work in the way that you describe, especially in the merging case. Generating the report would need to be a public function which returns the report object, and then Eldev would need to call / not call the merging function that accepts the report back. There would also need to be a function which checks if there is an existing report to merge? In all of these, there would need to be one such function per report type (in which case Eldev would need to know about all report types?), or make each of these switch by the report type again and call the report type's implementation function (nothing for the report types that can't do merging). That doesn't even include the report file list, and I still don't fully understand the exact requirements for that one. All in all this definitely doesn't look like an improvement, especially for the typical use case of Undercover, which doesn't really align with Eldev's.

[CyberShadow]

* In particular, the integration plugin contains these lines:

Wouldn't a better place for that be undercover--load-file-handler? That way, the message would directly precede the actual load/instrumentation, so that if there's an error during instrumentation, the log line (containing the file path) is directly before the error.

[doublep]

Issuing in undercover--load-file-handler would result in a message per file, as I understand, while currently I issue only one for all files combined.

[CyberShadow]

That's slightly misleading, as it will include files which are never actually loaded. Is there a strong reason to stick to the current behavior?

[doublep]

Current message says "instrumenting [now]", but yeah, you are right, I didn't think about instrumentation happenning lazily. Still, I'd rather give a single message to the tune of "coverage information will be collected for the following files: ...". I'd prefer to avoid intermixing real output with non-important debug messages, which can happen if such messages come from the file handler.

[doublep]

In other words, I'd rather leave single message, only rephrase it to be not misleading.

[CyberShadow]

Doesn't Eldev already have the list of files, considering the plan is to use (:files ...)? So, moving the message to before the call to the undercover macro would have the same effect?

[doublep]

Not if undercover disables itself because of not being on CI etc. There is no public way to find this out as far as I can see. But if there was, then yes, I could just log myself, making it easier.

[CyberShadow]

Yes, I see.

I added a message to undercover--edebug-files, similar to Eldev's wording.