[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Integration of dictionary package
From: |
Stefan Kangas |
Subject: |
Re: Integration of dictionary package |
Date: |
Thu, 19 Nov 2020 07:22:15 -0800 |
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"?
- Re: Integration of dictionary package, Matthias Meulien, 2020/11/08
- Re: Integration of dictionary package, Torsten Hilbrich, 2020/11/19
- Re: Integration of dictionary package, Torsten Hilbrich, 2020/11/19
- Re: Integration of dictionary package, Jean Louis, 2020/11/19
- Re: Integration of dictionary package, Stefan Kangas, 2020/11/19
- Re: Integration of dictionary package, Eli Zaretskii, 2020/11/19
- Re: Integration of dictionary package, Jean Louis, 2020/11/19
- Re: Integration of dictionary package, Eli Zaretskii, 2020/11/19