emacs-lisp-style-guide icon indicating copy to clipboard operation
emacs-lisp-style-guide copied to clipboard

Use descriptive keyword argument instead of `t` when boolean flag is expected

Open Fuco1 opened this issue 9 years ago • 17 comments

For example, functions which expects 5 arguments, all of which are t can get very confusing with calls like (foo t nil t nil t). It is not obvious what to do.

Recently I've been naming such arguments with descriptive names and then pass :argument-name keyword as a value. For example

(defun update (&optional force)
   "Update the list of values. If FORCE is non-nil, invalidate cache first.")

You would then call it (update) or (update :force) instead of (update t).

I think there is very little confusion possible over keyword-argument-functions and keyword-as-flags usage, especially when you can consult the documentation in about 1 second.

What do you think people?

Fuco1 avatar Sep 09 '14 17:09 Fuco1

I really like doing this too. It makes code much easier to understand when you don't have to visit every function's documentation to check what all those ts stand for.

That said, it's (sadly) not a practice I see used very often (do correct me if I'm wrong). Should the guide recommend something not widely used?

Malabarba avatar Sep 09 '14 20:09 Malabarba

@Fuco1 @Bruce-Connor I'd use (update 'force), though. It's not any better than :force, but avoids confusion with keywords, and—more importantly—it's the style that seems to be used throughout Emacs itself, and imho we should follow that in this style guide.

swsnr avatar Sep 12 '14 22:09 swsnr

Agreed

Malabarba avatar Sep 13 '14 07:09 Malabarba

I like @lunaryorn's suggestion.

bbatsov avatar Sep 15 '14 08:09 bbatsov

A somewhat related rule would be to avoid naming prefix arguments ARG or PREFIX.

bbatsov avatar Sep 15 '14 08:09 bbatsov

And how else would you call the prefix argument? I don't know, prefix argument is rarely supplied from lisp (non-interactively), and when, 99% of the time it means either "repeat this thing this many times" or "toggle if non-nil". Calling it a repeat or toggle is not adding much.

Fuco1 avatar Sep 15 '14 09:09 Fuco1

@Fuco1 Well, I do think that these names indeed add something: They convey what the prefix argument is used for, i.e. whether as toggle or boolean flag or for repetition, and that in turn helps users of the function, because the signature is now clear: I wouldn't need to read the docstring now in order to find out whether the prefix argument toggles or repeats.

Imho, as a general rule, you should always prefer a specific name about a generic one, even if the context of the generic name (the use as prefix argument) lets the user generally infer the meaning of the generic name. It's just easier for users.

swsnr avatar Sep 15 '14 09:09 swsnr

Using TOGGLE as the example.

  • If you're writing elisp, an argument named PREFIX tells you nothing, while an argument named TOGGLE says something.
  • If you're invoking interactively, PREFIX tells you it's the prefix argument (which is useful) without saying what it does, while TOGGLE tells you what it does but does't inform you that it's a prefix argument. So in both cases you'll have to read the rest of the doc.

So a specific name is more useful overall. You can also change the function's calling convention so that the argument is displayed as TOGGLE-PREFIX in the documentation (which is maximally descriptive), without changing its name in the actual code, but that seems like overkill.

Malabarba avatar Sep 15 '14 12:09 Malabarba

Ping :-)

bbatsov avatar Jan 29 '15 08:01 bbatsov

One advantage of :foo over 'foo is that they are fontified differently. For about a year now I use a special rule to fontify constants, such as 'foo so that helps (for me personally).

I don't mind either way, :arg or 'arg is fine, I would still slightly prefer :arg.

Fuco1 avatar Aug 10 '16 13:08 Fuco1

@bbatsov Is this project still alive? Can you assign this to me, I'll add a note somewhere about this. Let's go with the 'force style, it is used in Emacs itself which is a good point and worth following.

Fuco1 avatar Apr 12 '18 10:04 Fuco1

Yeah, it is. And every month I plan to come back to it and write some many things, but I'm buried with work, life and side projects. I've made you a collaborator, so feel free to make things happen! :-)

bbatsov avatar Apr 12 '18 13:04 bbatsov

@bbatsov I have a secret project writing an elisp static analyser kind of thing... so I would eventually like to include these rules as a plugin there. But I'm also not making a lot of progress (https://github.com/Fuco1/Elsa)

Fuco1 avatar Apr 12 '18 13:04 Fuco1

Nice!

And I know that @gonewest818 has been working on reviving elint recently. I'd be happy to help as well, but that's unlikely to happen any time soon.

bbatsov avatar Apr 13 '18 08:04 bbatsov

@Fuco1: fwiw, I adopted elisp-lint from the original author (who has since moved onto other things) and did a little spring cleaning on it a few months back. You'll see it's fairly limited in what it does -- either doing string matches on the text file or, in some cases, invoking existing emacs "infrastructure" packages like bytecomp, checkdoc, check-declare, or package to validate the code. There's definitely room for more/better tools in this area. If you see anything that can/ought to be changed in elisp-lint please let me know.

gonewest818 avatar Apr 18 '18 23:04 gonewest818

@gonewest818 I have a completely different approach. What I am doing is reading the forms, parsing the structure and actually running type inference to see what is going on. You can annotate the forms where types are ambiguous to help the engine to do a better job. You get a lot of information like the scope, variables, globals, functions and all sorts of other things at each point in the AST so the decisions are much more informed.

So this will be 100% syntactically based which allows for massively broader class of checks. The linting stuff would only piggyback on the existing infrastructure but that's not the main goal.

Fuco1 avatar Apr 19 '18 09:04 Fuco1

@Fuco1 yep, got it.

gonewest818 avatar Apr 19 '18 21:04 gonewest818