Allow ignoring of errors

[dajva]

Thanks for a great package.
Have you considered something like a white list or ignore list to allow specific constructs that violates some rule in package-lint?

This would allow to check major part of a package and new additions while keep older violating constructs for backwards compatibility.

I've found #94 but that was closed since the author changed his package, it seems.

[purcell]

Thanks for asking! I'm not keen on this approach, really. package-lint is made for package authors, who therefore have the ability to change their code freely. The rules in package-lint are not opinionated: they follow the elisp conventions, or detect compatibility issues, so it doesn't make sense to be able to disable them IMO.

[dajva]

Ok, thanks for the clarification.

[noctuid]

Not allowing disabling errors assumes that package-lint is perfect, and it isn't (and neither is any other linter I've ever used). It gives a lot of false positives. I'd love to be able to fail a build if linting fails, but I can't do that if there isn't some way to silence incorrect warnings. Please reconsider this if it is feasible to add something like per-line ignore comments.

Here is just one example of invalid warnings:

2541:1: error: "use-package-normalize/:general" doesn't start with package's prefix "general".
2541:1: error: `use-package-normalize/:general' contains a non-standard separator `/', use hyphens instead (see Elisp Coding Conventions).

It is impossible to both integrate correctly with use-package and not have these errors. Package lint complains if / or : are in a function name even if they aren't actually used as an alternative separator to -. In this case you could maybe argue against using /, but I think it gives some visual clarity, and there is nothing I can do about it in my package.

[purcell]

It is impossible to both integrate correctly with use-package and not have these errors.

I can see how that's an issue for conventions like use-package-normalize- etc. There are exceptions for some such common ones coded into package-lint, and I would consider adding this one.

In this case you could maybe argue against using /, but I think it gives some visual clarity, and there is nothing I can do about it in my package.

One key goal of package-lint is exactly to combat the "I think X is better than the convention" argument, but if use-package mandates it then I can see that it is problematic in this specific case.

[noctuid]

It's not a matter of convention. I'm fine with (most of) package-lint's conventions, but some package-lint warnings/errors gives are for things that are actually fine. Separators is just one example. If a warning is wrong, and I can't mark that I've looked at it and confirmed it's wrong/irrelevant, then I can't use package-lint automatically. If I can't use package-lint automatically, it is much less useful. Even using it with flycheck or manually, incorrect warnings add a lot of noise. I think it's impossible for package-lint to be perfect at detecting violations. This is why most linters let you ignore errors/warnings.

[purcell]

Thank you, yes, I do understand what you mean. I'm just not going to rush into making it possible to turn off warnings if they can instead be made more reliable.

are for things that are actually fine. Separators is just one example.

Are you saying you think / separators are actually fine in the general case, or that they should be accepted if they are a namespacing convention for callbacks of a package like use-package? I might agree with the latter.

This is why most linters let you ignore errors/warnings.

That's one theory. Another is that many linters let you ignore errors/warnings because individual companies have disparate standards.

[noctuid]

Thank you, yes, I do understand what you mean. I'm just not going to rush into making it possible to turn off warnings if they can instead be made more reliable.

Even if I was certain this was possible, I don't think I would agree with not allowing ignoring warnings. Let's say it's possible for package-lint to be perfect and then reliably be perfect forever. Before it is, it can't be used reliably to fail a CI run if there are any false positives.

Coming at it from the perspective of having to deal with other packages that don't meet the package-lint conventions, is it really better to add exceptions for specific packages than to just allow ignoring warnings? What is the harm in supporting this? For the use case of a MELPA reviewer looking at packages, package-lint could just support showing all warnings regardless of configuration. For packages already on MELPA (or that won't be on MELPA), developers can already just ignore some package-lint errors if they don't like its conventions already. Allowing silencing them doesn't affect enforcing the conventions. Again, this is not what I want to do. I want this functionality so that I can more easily ensure my code meets package-lint conventions. I don't understand the reasoning against allowing this (unless the concern is the difficulty/time needed).

Here are some more examples where I don't see how it would be possible to be perfect:

• "warning: `with-eval-after-load' is for use in configurations, and should rarely be used in packages." - rarely, but not never right? (e.g. handling optional dependencies; don't see how it would be possible to determine intention here)
• same as above for eval-after-load - for example, I have an alternate implementation of with-eval-after-load
• not sure you would handle all edge cases involving the "doesn't start with package's prefix" errors
  • defining a new use-package keyword (or something similar; not sure how package-lint could detect intention here)
  • the package uses define-namespace, so no prefixes are explicitly specified (maybe possible to address in package-lint)
  • I have an old package on MELPA that uses a different prefix in the defgroup from the filename and don't want to break backwards compatibility (guessing is possible to address in package-lint)
  • I'd rather use the same package prefix in test files (guessing is possible to address in package-lint)
• separator warning - there seems to be too many possible edge cases and no way to determine intention
• installable errors - if I have a package that isn't in a package archive and has multiple files (one which requires another)
• ".emacs.d" could be in a docstring

Question:

• "warning: Use a properly versioned dependency on "X" if possible." - is there a best practice for the case where a package depends on a more recent version of a package than there is actually a version number for?

Are you saying you think / separators are actually fine in the general case, or that they should be accepted if they are a namespacing convention for callbacks of a package like use-package? I might agree with the latter.

I think not allowing / as a separator is fine. I have functions that have colons in them that are not separators but generate warnings as if they were. / is not necessarily a separator either.

That's one theory. Another is that many linters let you ignore errors/warnings because individual companies have disparate standards.

Sure, there are multiple reasons ignoring warnings is useful. It would be wrong to say that either is the only reason. Allowing completely ignoring a certain warning can be useful when there are differing conventions (but sometimes a warning is just broken too often to be useful). Ignoring warnings per line is most often useful for handling invalid messages; if it was just about standards, I don't think per line ignores would be very useful (but maybe there is some use case I haven't encountered). I've never seen a non-trivial linter that didn't generate incorrect messages even when following its standards.

Realistically, I don't see how this is fixable without allowing ignoring warnings. I guess you could pull out potentially imperfect messages into a separate category in order to not fail a CI run, but this would still be extra noise. From my perspective, I'd rather just be able to look at something like a prefix warning, confirm that there is no issue, mark it as reviewed, and then never see it again. In this scenario, I don't have to rely on package-lint being perfect, and package-lint doesn't have even have to try to handle something like trying to determine if a prefix might actually be valid in certain edge cases.

[purcell]

As of a couple of days ago you can optionally ignore warnings in CI builds.

[noctuid]

Thanks, that's an improvement, but a lot of the false positives I mentioned are currently errors and not warnings (e.g. prefix and separator errors). Ideally I'd like to have warnings fail the build though. This also doesn't help with the noise issue. Would you accept a PR adding support for optionally checking per-line ignores?

[noctuid]

If the goal is to try to promote the standard suggested by package-lint and discourage alternatives, how about only allowing per-line ignores for messages known not to be perfect? Would this be an acceptable compromise? I only care about dealing with false positives.

Example implementation: Add a variable for turning on per-line ignores. Add an optional argument to package-lint--error to provide a name for the rule. Check if a comment on the line of the error or a comment just before the line (which could span multiple lines) has something like package-lint-ignore: rule-name,rule2-name. Don't add the error to package-lint--errors if it is ignored and the per-line ignore variable is non-nil. Only give rule names to rules known to have false positives.

[dajva]

Adding my 2c. I stopped using this package because I can't keep my non conforming single function name that I want to keep for backwards compatibility. I think this is sad because this package is so useful and have improved my package considerably.
I think these kind of decisions is pretty common in any software project. You set up a set of goals but in the end you need to make compromises when these goals conflict. If you want to use this package you loose part of that freedom which in my case resulted in that I had to drop it.

With that said I think you should follow your vision for this package. If you change your mind though, I would gladly pick it up again.

[purcell]

Adding my 2c. I stopped using this package because I can't keep my non conforming single function name that I want to keep for backwards compatibility.

You can change the function name and use define-obsolete-alias for the old name: package-lint will (intentionally) be perfectly happy with that. So you might find that you could resume using package-lint.

[canatella]

I'm stumbling upon the with-eval-after-load warning and it indeed would be nice If I could ignore those. I don't think an ignore file is a good solution but if package-lint could understand some kind of comment before the warning and lower its level to less then warning it could be a good compromise:

• I would be forced to document why I'm doing the bad stuff.
• I could run package-lint when developing or on CI builds with a level >= warning, thus avoiding warning fatigue and getting that good feeling about being all clear.
• Melpa maintainers or any one reviewing the code could run package-lint with all levels and it would display the comments so they would immediately be able to check if it is indeed required or not.

Like the c/c++ checker clang-tidy is doing:

;; no-lint(with-eval-after-load-outside-configuration): it is used here for this and that reason...

[purcell]

I'm stumbling upon the with-eval-after-load warning and it indeed would be nice If I could ignore those.

Better would be if you share the code and perhaps I can suggest a better alternative. :-)

[canatella]

I'm stumbling upon the with-eval-after-load warning and it indeed would be nice If I could ignore those.

Better would be if you share the code and perhaps I can suggest a better alternative. :-)

I'd appreciate that :) https://github.com/canatella/xwidget-plus/blob/master/xwidget-plus-follow-link.el . My package augments ido, ivy and helm to browse link using xwidget-webkit. But I don't want to (require 'helm) if the user doesn't use helm. Using (when (featurep 'helm) ...) would mean that if a user loads my package before helm, he doesn't get the intended functionality. Also we are discussing that here: melpa/melpa#6755

[purcell]

My package augments ido, ivy and helm to browse link using xwidget-webkit. But I don't want to (require 'helm) if the user doesn't use helm. Using (when (featurep 'helm) ...) would mean that if a user loads my package before helm, he doesn't get the intended functionality. Also we are discussing that here: melpa/melpa#6755

That's actually quite a clear case for small separate packages which actually have the appropriate dependencies. You could still keep them in the same repo, so - for example - you'd have an xwidget-plus-helm.el or similar, which would depend on xwidget-plus and helm. Then the question of featurep / with-eval-after-load doesn't even arise because the code is just packaged conventionally. I appreciate that in some cases the code specific to Ivy or Helm is quite brief, but packaging is super cheap, and it's the right thing to do. (Obviously, since ido is built in, separate packaging is not necessary for the corresponding code.)

[rnkn]

In order to maintain compatibility with Emacs 24 I've added outline-* function aliases when emacs-major-version < 25. The problem is I get both

error: Aliases should start with the package's prefix "PREFIX"

and

error: You should depend on (emacs "25.1") if you need `outline-*'

What should I do in this situation?

[purcell]

@rnkn The elisp coding conventions document covers this -- search for the example referring to gnus-point-at-bol.

[rnkn]

@purcell thanks!

[noctuid]

That's actually quite a clear case for small separate packages which actually have the appropriate dependencies. You could still keep them in the same repo

Is the correct thing in this case to also have them be separate recipes on MELPA?

I'm trying to use package-lint for CI again. I am developing a new package, so I don't have to worry about maintaining backwards-compatibility and will switch to separate packages for previously optional dependencies. I am still blocked by the use-package issue though. This results in 4-6 errors per use-package keyword, and I am writing a package that adds a lot of use-package keywords. To deal with use-package's extension system, can the prefix and separator checkers specifically look for use-package-handler/:, use-package-normalize/:, and use-package-autoloads/: (or some customizable list). If not, do you have another suggestion for how to handle this?

Some more minor questions:

• general-extended-def-:which-key -: is not being used as a separator here; can package-lint check if : is preceded by -?
• Regarding eval-after-load, what should I do if I actually want to defer code that can have some cost? For example, general.el records user-defined keybindings in a way that they can be later be displayed in tables in an org-mode buffer. The recording takes extra time and is never needed until the user actually wants to show the keybindings. Even though the supporting library will be a mandatory dependency for the package, I'd like to have an option to defer the recording until the supporting library is actually intentionally loaded by the user to display the recorded information.

[purcell]

Is the correct thing in this case to also have them be separate recipes on MELPA?

Yes

I'm trying to use package-lint for CI again. I am developing a new package, so I don't have to worry about maintaining backwards-compatibility and will switch to separate packages for previously optional dependencies. I am still blocked by the use-package issue though. This results in 4-6 errors per use-package keyword, and I am writing a package that adds a lot of use-package keywords. To deal with use-package's extension system, can the prefix and separator checkers specifically look for use-package-handler/:, use-package-normalize/:, and use-package-autoloads/: (or some customizable list). If not, do you have another suggestion for how to handle this?

Yes, potentially. See package-lint--sane-prefixes. Would consider a PR.

Some more minor questions:

* `general-extended-def-:which-key` -`:` is not being used as a separator here; can package-lint check if `:` is preceded by `-`?

Maybe? But the example is a bit odd, I feel like I'm missing context. Is that another upstream package symbol naming convention?

* Regarding `eval-after-load`, what should I do if I actually want to defer code that can have some cost?  For example, general.el records user-defined keybindings in a way that they can be later be displayed in tables in an org-mode buffer.  The recording takes extra time and is never needed until the user actually wants to show the keybindings.  Even though the supporting library will be a mandatory dependency for the package, I'd like to have an option to defer the recording until the supporting library is actually intentionally loaded by the user to display the recorded information.

If I'm understanding this correctly, you could put the recording functionality at the top level of a separate file, and put the "display this thing in a buffer" command in the same file, autoloaded.

[noctuid]

Yes, potentially. See package-lint--sane-prefixes. Would consider a PR.

Thanks, I can make a PR.

Is that another upstream package symbol naming convention?

It was my naming convention. This is not a big deal though because I'm rewriting the package and will use a saner extension mechanism rather than following use-package's lead for how to add new keywords again.

If I'm understanding this correctly, you could put the recording functionality at the top level of a separate file, and put the "display this thing in a buffer" command in the same file, autoloaded.

Right now, the recording and display functions are both in a separate package. The display command is autoloaded. The recording functionality is meant to be used in functions that will run in a user's init.el, but I don't want to actually load the package or do any recording until the user needs it.

This is not a real example, but hopefully it demonstrates what I mean:

;; my package provides something like this
(defun my-define-key (keymap key definition)
  (with-eval-after-load 'record-things
    (record-things keymap key definition))
  (keymap-set keymap key definition))

;; user's init.el will call my-define-key

That's currently how it's done to prevent recording taking up extra time during initialization for the user's key definitions (since recording involves some extra processing). I can't think of a better way around this. I could use a hook or store the information in some other way, but that doesn't really seem better.

[purcell]

Okay, but that means someone else needs to load record-things, and that you might get lots of record-things function invocations (one for each key defined) at that point etc. It's all very indirect IMO. If the command needs access to the recorded data then it should call a function to "ensure the data has been recorded", and that function should do the work on demand if necessary.

[noctuid]

record-things would be loaded automatically whenever the user called the describe command. They are part of the same package. There will always be one record-things call for each key regardless of when it is loaded.

If the command needs access to the recorded data then it should call a function to "ensure the data has been recorded", and that function should do the work on demand if necessary.

If the command is running, then the package will have already loaded, and the data will have been recorded. I'm not sure I made it clear that there is a single package doing both the recording and the display/describing (no external packages access the recorded data). Is your suggestion to temporarily store the information in a variable, and then later call a function to record and clear the stored information from that variable?

[monnier]

That's actually quite a clear case for small separate packages which actually have the appropriate dependencies. You could still keep them in the same repo, so - for example - you'd have an xwidget-plus-helm.el or similar, which would depend on xwidget-plus and helm. Then the question of featurep / with-eval-after-load doesn't even arise because the code is just packaged conventionally. I appreciate that in some cases the code specific to Ivy or Helm is quite brief, but packaging is super cheap, and it's the right thing to do. (Obviously, since ido is built in, separate packaging is not necessary for the corresponding code.)

As you noticed with ido whether the package is installed or not is mostly irrelevant, so adding to Package-Requires: (with or without splitting into a separate package) doesn't help. The problem is that package FOO needs to modify some variable of package BAR but it wants to do it lazily so that loading package FOO doesn't force package BAR to be loaded at the same time.

You can argue that using with-eval-after-load is still a sign of poorly written code, and I'll largely agree with you, but in such a case the poorly written code is in BAR which should be designed in such a way that FOO can do its thing without needing with-eval-after-load (typically by setting a variable/property/function/younameit; nowadays a cl-defmethod might also be a good way to go about it), but flagging the use of with-eval-after-load is pointing the finger at the victim rather than the culprit.