AdamNiederer / cov

An emacs extension for displaying coverage data on your code
GNU General Public License v3.0
76 stars 16 forks source link

Cov does not like invalid .info files #45

Closed xendk closed 2 years ago

xendk commented 2 years ago

If cov finds an .info file, but it turns out to not be a coverage file, it throws an error which can break things in odd ways. I was wondering why .dir-locals.el wasn't working, turns out cov found the .info files that's a requirement in Drupal 7 modules, barfed and apparently stopped .dir-locals.el from being used.

cov-mode shouldn't throw any errors for this reason, but just complain in the minibuffer.

snogge commented 2 years ago

Could you give me an example of a file setup that is problematic and steps to reproduce? I'm afraid your description was not enough for me to understand the problem.

xendk commented 2 years ago

OK, I automatially enable cov for some languages:

(use-package cov
  :custom-face
  (cov-heavy-face ((t (:foreground "green"))))
  (cov-light-face ((t (:foreground "orange"))))
  (cov-none-face ((t (:foreground "red"))))
  :hook
  ((emacs-lisp-mode php-mode js-mode) . cov-mode)
  :straight t)

In php-mode I also add a hack-local-variables-hook:

  (php-mode . (lambda ()
                (add-hook 'hack-local-variables-hook #'xen-php-setup-tools nil t)))

xen-php-setup-tools checks whether some tools (specifically phpcs and phpstan) is installed in the project using composer and sets flycheck variables to point at them if found. I'm using hack-local-variables-hook because that's explicitly run after file local variables has been set, and I want to be able to set a variable for my function in .dir-locals.el in case the project uses another path for installed tools than the default vendor/bin. It would of course be a neater solution to run composer and ask it what the bin-dir is, but that a bit more involved solution, as one would want to add caching to avoid a composer invocation on each file open, and it's a bit much work considering I only have exactly one project that does this. But I digress.

When cov raises an error, this apparently causes the xen-php-setup-tools to never be called. I ended up here because flycheck couldn't find phpcs/phpstan, which I debugged to the fact that my variable for where the bin dir was, was still at the default value. Opening any file with a mode that didn't activate cov-mode (.md file for instance) would properly set the variable according to .dir-locals.el.

So it would seem that the error kills at least .dir-locals.el, if it causes any more breakage, I don't know. I've created a ]test case](https://github.com/AdamNiederer/cov/files/8459834/test.zip) in that you'll find a directory with files demonstrating the issue.

snogge commented 2 years ago

@xendk, can you check whether #46 solves the problem for you?

xendk commented 2 years ago

@snogge Yes, that seems to fix the problem. :+1:

A note on implementation though: I don't think mode functions are supposed to throw errors, but rather inform the user that things didn't go according to plan. This issue seems to suggest that Emacs doesn't try to catch errors and carry on, which causes one unhappy minor-mode to break the chain. But I'm not an expert on such matters.

snogge commented 2 years ago

It's a good point. I should probably update the lcov parser to handle that. But even if the parser didn't error, it is good to have a check whether found files are lcov trace files. Otherwise the first found file would block a later correct file. It would probably be a good idea to have some sort of mapping between the major mode of the source file and possible coverage data formats. That way we could avoid trying to load irrelevant config data files.

xendk commented 2 years ago

But even if the parser didn't error, it is good to have a check whether found files are lcov trace files. Otherwise the first found file would block a later correct file.

Oh, good point.

It would probably be a good idea to have some sort of mapping between the major mode of the source file and possible coverage data formats. That way we could avoid trying to load irrelevant config data files.

Well, some formats are pretty generic, so having to maintain a mapping might be annoying. I'd say that a quick test that the first few lines of a candidate file has the right format is enough. For files with a pretty uniqe filename (clover.xml for instance) I don't think it's worth the bother.