GNU bug report logs - #49245
Enchant dictionaries list not being correctly set, and other minor fixes

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49245 in the body.
You can then email your comments to 49245 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Reuben Thomas <rrt <at> sc3d.org>
To: bug-emacs <bug-emacs <at> gnu.org>
Subject: Enchant dictionaries list not being correctly set,
 and other minor fixes
Date: Sun, 27 Jun 2021 22:20:10 +0100

[Message part 1 (text/plain, inline)]

I found a bug recently where, when using Enchant as the back-end for
ispell.el, spellchecking was locking up for some languages.

This turned out to be the combination of two problems.

First, ispell-find-enchant-dictionaries was incorrectly merging
ispell-dictionary-base-alist into its result. This caused the
ispell-set-spellchecker-params to fail to add the correct "-d LANG" flag
arguments to the list of dictionaries that it used to start Enchant, which
in turn meant that the process was started as e.g.

enchant-2 -d francais # rather than -d fr_FR

and failed to start properly. I have fixed this by simply removing the
incorrect code (patch 0003 attached).

Secondly, ispell.el failed to notice that it had not actually started an
Enchant process, and hung while trying to read from it in
ispell-accept-output. I fixed this by testing that the process is live
before trying to read from or write to it (patch 0002 attached).

Finally, while reading the source code I found an ancient comment that is
more of a commit message in spirit (it explains how the current code came
to be that way, rather than explaining something about how it works), so I
removed it (patch 0001 attached).

As usual with my infrequent patches, I would appreciate other eyes on them
before I install them, if possible. I remain an active user of ispell with
Enchant, though, so I will give them plenty of manual testing. Thanks in
advance!

-- 
https://rrt.sc3d.org

[Message part 2 (text/html, inline)]

[0002-lisp-textmodes-ispell.el-Check-process-is-live-befor.patch (text/x-patch, attachment)]

[0003-lisp-textmodes-ispell.el-Fix-finding-dictionaries-fo.patch (text/x-patch, attachment)]

[0001-lisp-textmodes-ispell.el-ispell-word-Remove-a-redund.patch (text/x-patch, attachment)]

Message #8 received at 49245 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Reuben Thomas <rrt <at> sc3d.org>
Cc: 49245 <at> debbugs.gnu.org
Subject: Re: bug#49245: Enchant dictionaries list not being correctly set,
 and other minor fixes
Date: Mon, 28 Jun 2021 15:13:54 +0300

> Date: Sun, 27 Jun 2021 22:20:10 +0100
> From:  Reuben Thomas via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> I found a bug recently where, when using Enchant as the back-end for ispell.el, spellchecking was locking up
> for some languages.
> 
> This turned out to be the combination of two problems.
> 
> First, ispell-find-enchant-dictionaries was incorrectly merging ispell-dictionary-base-alist into its result. This
> caused the ispell-set-spellchecker-params to fail to add the correct "-d LANG" flag arguments to the list of
> dictionaries that it used to start Enchant, which in turn meant that the process was started as e.g.
> 
> enchant-2 -d francais # rather than -d fr_FR
> 
> and failed to start properly. I have fixed this by simply removing the incorrect code (patch 0003 attached).
> 
> Secondly, ispell.el failed to notice that it had not actually started an Enchant process, and hung while trying to
> read from it in ispell-accept-output. I fixed this by testing that the process is live before trying to read from or
> write to it (patch 0002 attached).
> 
> Finally, while reading the source code I found an ancient comment that is more of a commit message in
> spirit (it explains how the current code came to be that way, rather than explaining something about how it
> works), so I removed it (patch 0001 attached).
> 
> As usual with my infrequent patches, I would appreciate other eyes on them before I install them, if possible.

OK for the first 2 patches, but please don't remove that comment, it
explains something important, and there's no reason to remove it (or
many other similar comments we have throughout our code).

Thanks.

Message #11 received at 49245 <at> debbugs.gnu.org (full text, mbox):

From: Reuben Thomas <rrt <at> sc3d.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 49245 <at> debbugs.gnu.org
Subject: Re: bug#49245: Enchant dictionaries list not being correctly set, and
 other minor fixes
Date: Mon, 28 Jun 2021 13:31:01 +0100

[Message part 1 (text/plain, inline)]

On Mon, 28 Jun 2021 at 13:13, Eli Zaretskii <eliz <at> gnu.org> wrote:

> OK for the first 2 patches,


Thanks for the review.


> but please don't remove that comment, it
> explains something important, and there's no reason to remove it (or
> many other similar comments we have throughout our code).
>

In that case, please can you explain it, and I can rewrite it so that its
significance is more evident. As I said, it explains why something was
changed in the past (which is useful information in a commit message)
rather than how or why the current code does something that may not be
obvious just from reading the code (which would be suitable for a comment).
Commit f0a1f8bdb5, which introduces it, has the message "Do not ignore
short words". The current code does not have to *do* anything to check
short words; that commit simply removed a check. I do not see anything in
the current code that raises any questions that need answering by a
comment. On the contrary, the comment raises a question: "is there some
setting for minimum word length that I need to be aware of?". So I feel
I've missed something here that a rewording of the comment could fix.

-- 
https://rrt.sc3d.org

[Message part 2 (text/html, inline)]

Message #14 received at 49245 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Reuben Thomas <rrt <at> sc3d.org>
Cc: 49245 <at> debbugs.gnu.org
Subject: Re: bug#49245: Enchant dictionaries list not being correctly set, and
 other minor fixes
Date: Mon, 28 Jun 2021 17:37:01 +0300

> From: Reuben Thomas <rrt <at> sc3d.org>
> Date: Mon, 28 Jun 2021 13:31:01 +0100
> Cc: 49245 <at> debbugs.gnu.org
> 
>  but please don't remove that comment, it
>  explains something important, and there's no reason to remove it (or
>  many other similar comments we have throughout our code).
> 
> In that case, please can you explain it, and I can rewrite it so that its significance is more evident. As I said, it
> explains why something was changed in the past (which is useful information in a commit message) rather
> than how or why the current code does something that may not be obvious just from reading the code
> (which would be suitable for a comment). Commit f0a1f8bdb5, which introduces it, has the message "Do
> not ignore short words". The current code does not have to *do* anything to check short words; that commit
> simply removed a check. I do not see anything in the current code that raises any questions that need
> answering by a comment. On the contrary, the comment raises a question: "is there some setting for
> minimum word length that I need to be aware of?". So I feel I've missed something here that a rewording of
> the comment could fix.

It is customary to leave a comment when we delete some code, but are
not 100% sure that code was a clear mistake.  Since deleting code
leaves nothing behind (unlike if you add or change code), the comment
serves as a kind of "trace" for what once was there, but is no more.

If you want a practical case where this could be useful, imagine a bug
report regarding special treatment of words shorter than 3 letters
(which are normally ignored).

Message #17 received at 49245 <at> debbugs.gnu.org (full text, mbox):

From: Reuben Thomas <rrt <at> sc3d.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 49245 <at> debbugs.gnu.org
Subject: Re: bug#49245: Enchant dictionaries list not being correctly set, and
 other minor fixes
Date: Mon, 28 Jun 2021 18:11:32 +0100

[Message part 1 (text/plain, inline)]

On Mon, 28 Jun 2021 at 15:37, Eli Zaretskii <eliz <at> gnu.org> wrote:

> It is customary to leave a comment when we delete some code, but are
> not 100% sure that code was a clear mistake.  Since deleting code
> leaves nothing behind (unlike if you add or change code), the comment
> serves as a kind of "trace" for what once was there, but is no more.
>

Thanks for the explanation, sounds like a culture of commenting previously
unfamiliar to me.

I will close this bug now.

-- 
https://rrt.sc3d.org

[Message part 2 (text/html, inline)]

Message #22 received at 49245-done <at> debbugs.gnu.org (full text, mbox):

From: Reuben Thomas <rrt <at> sc3d.org>
To: 49245-done <at> debbugs.gnu.org
Subject: Closing
Date: Mon, 28 Jun 2021 18:12:20 +0100

[Message part 1 (text/plain, inline)]

Fixed by 7c93009d11 and 881e75873d.

-- 
https://rrt.sc3d.org

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.