Re: Integration of dictionary package

[Stefan Kangas]

Torsten Hilbrich <emacs.nolkaf@hilbrich.tk> writes:

> I have now completed the work on my branch
> feature/integration-of-dictionary-el. For reference, here is a list of
> the commits so far:

Do you have any unit tests for this stuff?  If you do, it would be nice
if we could bring that over too.

I don't use this stuff myself (yet?), so I unfortunately can't provide
substantial feedback.  But please find some minor comments and nits
mostly regarding our coding conventions below.

> +;;; dictionary-connection.el --- TCP-based client connection for dictionary

Any chance we could use lexical-binding:t here?

> +;; the Free Software Foundation; either version 2, or (at your option)

Should be version 3.

> +;; any later version.
> +
> +(defmacro dictionary-connection-p (connection)

Would it make sense to change this and the ones below it into defuns?

> +(defun dictionary-connection-create-data (buffer process point)
> +  "Create a new connection data based on `buffer', `process', and `point'."

Our convention is to format docstrings like:

    "Create a new connection data based on BUFFER, PROCESS and POINT."

> +(defun dictionary-connection-status (connection)
> +  "Return the status of the connection.
> +Possible return values are the symbols:
> +nil: argument is no connection object
> +'none: argument has no connection
> +'up: connection is open and buffer is existing
> +'down: connection is closed
> +'alone: connection is not associated with a buffer"
> +  (if (dictionary-connection-p connection)

Very minor nit: If you change this to a `when', you don't need the nil
at the end.

> +;;; dictionary.el --- Client for rfc2229 dictionary servers

Same as above: Any chance we could use lexical-binding:t here?

> +  (make-local-variable 'dictionary-data-stack)
> +  (setq dictionary-data-stack nil)

You could use `setq-local' here instead.

> +         (error "Dictionary \"%s\" not existing" dictionary)

How about "does not exist"?