[Request for Review] Emacs Introspection System

Hi there, this is a **request for reviews of [eis.el](https://github.com/bkaestner/eis.el)**, a new (pre-release) package to get lists of your currently bound functions, variables, faces and known system fonts. This package also contains my first usage of `(cl-)defmacro` and is my largest ELisp package yet with ~400LoC*. I'd like to take this opportunity to get some feedback from you all, both in terms of functionality as well as in terms of the code style. Feel free to point out anything here in the comments or within GitHub issues, thanks! PS: Since it's based on `tabulated-list-mode` it should work fine with [`tablist`](https://github.com/politza/tablist) and similar packages. ^(* You might scoff at that number, but then you're probably the perfect reviewer for this project (:)

[deleted]

Looks nice! Please publish it at some point in MELPA :)

I am always excited when I see words like introspection, reflection, compiler and language, so I have spent few minutes with your code. > in terms of the code style. To start with, I recommend you to run checkdoc and package-lint on your code. Actually, do everything outlined in [Melpa contributing](https://github.com/melpa/melpa/blob/master/CONTRIBUTING.org), even if you don't plan to contribute your package to Melpa. These are good general guidelines for any package. It is annoying to get compiler warning for doc string wider than 80 chars. Same for the code. Keep yourself to 80 lines. For example, I personally have 4k screen, 3840x2160. I normally, for like 99% of time, split it in two vertical halves, right is Emacs, left is Firefox. Then I split my Emacs frame in two vertical windows, one to left is the code, and one to right are either some other buffers, or split into few horizontal windows for supporting buffers like help, shell, messages etc. If lines are wider than 80 chars, lines get wrapped which looks ugly and hard to read. Even if we live in a time with big screens, screen space is still somehow tight. > in terms of functionality On my machine, eis-show-variables results with this error message: helm-M-x-execute-command: Invalid key sequence in substitution: ‘cid:" which only matches parts embedded to the Multipart/Related type MIME contents and Gnus will never connect to the spammer’ I don't know if its just my setup or your code, but that is what I get, and the list is empty. eis-show-functions is just very slow. I haven't looked in details why, I meant to, but I didn't get to it. I have though glanced over the code and saw some help functions. Here is a little thing: (defun eis--elt< (a b) "Compare the first elements within A's and B's cadr case-insensitive." (setq a (elt (cadr a) 0)) (setq b (elt (cadr b) 0)) (string< (downcase (if (listp a) (car a) a)) (downcase (if (listp b) (car b) b)))) Are you sure you need this downcasing? I think you can safely compare those without downcasing; at least I see no visible difference, neither in order nor in size of the result: (defun eis--elt< (a b) "Compare the first elements within A's and B's cadr case-insensitive." (let ((a (elt (cadr a) 0)) (b (elt (cadr b) 0))) (string< (if (listp a) (car a) a) (if (listp b) (car b) b)))) Check out elisp documentation for [display](https://www.gnu.org/software/emacs/manual/html_node/elisp/Selective-Display.html) and string comparison. You don't need to "handle" ellipsis on your own. Instead, use text properties and hidden text and Emacs will automatically display ellipsis. The pros of using built-in stuff is [if a user customize how the ellipses looks like](https://www.gnu.org/software/emacs/manual/html_node/elisp/Selective-Display.html), even your package will automatically be displayed with the user's choice. In Emacs, text processing in buffers is much faster than with strings. Considering how slow your eis-show-functions is I would consider trying to optimize the code somehow. For example, your eis--match-string is creating quite few temporary strings. Is trim-string at the beginning really needed? I see no difference if I comment it away. You also seem to be using that function in only two places, both of which seem to not need that one: (defun eis--format (fmt &rest args) "Format ARGS according to FMT on a single line." (apply #'format fmt args)) I see no difference, with *eis--ensure-single-line* or without. I am not 100% on that one, I have just glanced over results, check it yourself for sure. The other one *eis--shorten-docstring* is a bit interesting. It seems that the purpose is to simply cut string after some threshold and show ellipses. Is it not possible to just check the length of the doc string, and set the text properties for the rest of the string after some threshold, to invisible, and let Emacs do the ellipses for you? Then the entire exercise with strings in *eis--match-string* goes away; i.e. you can just remove the function. Another thing about *eis--match-string*, [it is not considered really a good practice to have two quite different concerns in same function](https://en.wikipedia.org/wiki/Separation_of_concerns). The function name and the doc say you are returning a match of a string and regexp. However, the code says something different, since you are returning a possibly modified and shortened string. Could you maybe do match in that function and do the string transformation itself in *eis--shorten-docstring* as you suggest with the name of that function? I don't know if I am missing something, perhaps I misunderstand your code, so take it with a grain of salt. I haven't looked more at the rest, this took already more time than what I meant it when I took a peek :-). Hope it helps and good luck with elisping!

Don't have time for a thorough review of this, but the screenshots look nice and I can see this being very useful. Thanks for contributing!