purcell / package-lint

A linting library for elisp package metadata
GNU General Public License v3.0
195 stars 34 forks source link

Allow The Implementation of Interfaces Provided by Other Packages #68

Open colonelpanic8 opened 7 years ago

colonelpanic8 commented 7 years ago

I have one package that provides an abstract interface which is to be implemented by objects written in other packages. This interfaces uses names which are prefixed with the name of the package which defines the interface for obvious reasons. This means that when I define a class which implements the interface in another package I get the "doesn't start with package's prefix" package-lint error.

Since this isn't really something that can be avoided, it seems that there should be some way to prevent package-lint from nagging about it.

purcell commented 7 years ago

for obvious reasons [snip] Since this isn't really something that can be avoided

Are you using eieo, cl-defmethod, or similar? Can you please link to an example so it's clearer what you're trying?

joewreschnig commented 7 years ago

One case I've run into is extending use-package with custom keywords. These must begin with use-package-normalize/ and use-package-handler/.

colonelpanic8 commented 7 years ago

Are you using eieo, cl-defmethod, or similar? Can you please link to an example so it's clearer what you're trying?

No. I just have one package that provides an abstract interface, and another that implements that interface. The names of the interface come from the package providing the interface, but the definitions occur in the package implementing the interface.

joewreschnig commented 7 years ago

@purcell, what do you think of replacing package-lint--sane-prefixes with a file-local list?

purcell commented 7 years ago

what do you think of replacing package-lint--sane-prefixes with a file-local list?

Hmmm, might be an option. Ideally I'd like to avoid complexity and optional checks in package-lint if possible.

colonelpanic8 commented 7 years ago

Are you using eieo, cl-defmethod, or similar?

Sorry, I misspoke here. I AM using cl-defmethod/eieio for this. I thought you were asking something else.

@purcell Do you have a solution in mind that would address this?

alexmurray commented 7 years ago

@purcell - an example of this is implementing an xref backend - xref uses cl-defmethod to do dispatch based on the name of the current backend - an example is from the xref-js2 package - https://github.com/NicolasPetton/xref-js2/blob/master/xref-js2.el#L87 - in this case there are errors -

"xref-backend-identifier-at-point" doesn't start with the package's prefix "xref-js2"

etc.

purcell commented 7 years ago

Right. I don't have a good general approach to tackle this, sorry. I don't want to go the route of file-local exception lists, ideally.

colonelpanic8 commented 7 years ago

@purcell Maybe I'm missing something, but the solution seems pretty obvious to me. Let me elaborate:

All calls to defmethod and cl-defmethod are associated with some class whose name will appear on their definition line. ex:

(cl-defmethod xref-location-marker ((l xref-file-location))

If we are careful to always check that defclass statements always have the name of the package they are defined in as a prefix, then we only really need to check that defmethods have the current package name provided that their associated class ALSO has the current package name as a prefix, because there would already be a flycheck issue if the associated defclass did not have the appropriate package name.

To put it concretely, the logic of the defmethod check should, IMO, look like this:

a) find the associated class name b) check if the class name matches the current package name c) if it does make sure that the method name ALSO has the current package name d) (MAYBE) if it doesn't, check, at the very least, that the class and the method share some prefix

colonelpanic8 commented 7 years ago

Hmmm I see that package-lint--get-defs depends on imenu to find the locations of definitions, and that imenu does not actually find class definitions, which means that classes are NOT actually checked. This seems like an issue in itself.

joewreschnig commented 7 years ago

It looks like the functions to search for come from lisp-imenu-generic-expression where defclass is matched as "Type" but package-lint only cares about "Variables" and "Defuns" (which I guess is the nil key).

I've noticed before that defalias also doesn't get caught; it doesn't seem to be listed at all. That seems reasonable if you're trying to navigate symbols, but bad for package-lint's case.

Maybe package-lint should provide its own imenu expression? That would let it include defalias, and ignore (cl-)defmethod.

purcell commented 7 years ago

Maybe package-lint should provide its own imenu expression? That would let it include defalias, and ignore (cl-)defmethod.

Yep, probably a good idea.