Re: ispell.el, flyspell.el: better ispell/aspell switching

[Agustin Martin <agustin.martin@hispalinux.es>]

On Wed, Apr 16, 2008 at 11:21:33AM -0400, Stefan Monnier wrote:
> 
> > ``distro-override-dicts-alist'' will override even dicts in
> > ``found-dicts-alist'' (currently the alist of parsed aspell dicts and
> > associated properties if spellchecker is aspell). Put here just in
> > case is ever needed, but I am currently not using it in Debian.
> 
> In what kind of circumstance would this be needed?

I put this there just for completeness, but I do not think of a particular
case where this is actually needed. The only thing I can think of is to
work around a bad aspell autodetection for a dict, but in this case is the
autodetection what needs a fix. Not a problem to remove it.
 
> > ``distro-fallback-dicts-alist'' will just override (or complement if
> > originally not present) initial ``ispell-dictionary-alist'' values
> > (those that are in ``base-dicts'' in the function), so an entry can be
> > fixed without patching ispell.el or waiting for a new emacs version
> > be released.
> 
> Can't this be done by:
> 1 - remove autoloads on ispell-dictionary-alist-N
> 2 - merge ispell-dictionary-alist-N into ispell-base-dictionary-alist
> 3 - let Debian use after-eval-hook to adjust ispell-base-dictionary-alist

(I guess you mean after-load-hook instead of after-eval-hook) Note that 
this will fail if spellchecker is switched after ispell.el is loaded. If so,
ispell-base-dictionary-alist will be left with the old values, that might
not match what the new available dicts (or new needs) for spellchecker.
 
> > For instance, this is the relevant part in debian-ispell.el
> 
> > ---------------------------
> > (defun debian-ispell-initialize-dicts-alist ()
> >   (setq distro-fallback-dicts-alist
> >         (if ispell-really-aspell
> >             debian-aspell-only-dictionary-alist
> >           debian-ispell-only-dictionary-alist)))
> 
> Why does Debian need to do that?  I.e. What kind of changes does
> this effect?  Why doesn't ispell.el get it right on its own?

Note that we use the same FSF ispell.el and flyspell.el (with a number of
compatibility patches and some known limitations) for emacs21, emacs22 and
xemacs, so dictionaries are integrated in a similar way for all emacs
flavours. Those are in a package different from the {x}emacs* that get
installed along with the ispell or aspell dictionaries.

The current way of doing things is that dictionary maintainers provide info
that is used to build a dictionary-alist for the really installed dicts
after the provided info. That makes debian-aspell-only-dictionary-alist for
the aspell dicts and debian-ispell-only-dictionary-alist for the ispell
dicts, and that is the info passed to the hook as
distro-fallback-dicts-alist.

One advantage of this is that there is no need to patch ispell.el each time
a new dictionary package is added to Debian or some info is fixed, and the
right dictionary-alists are automagically rebuilt every time a dictionary
package is installed in the user box. Note that for emacs and aspell this
would not be a big problem because of the autodetection, but is a problem
for emacs and ispell as well as for xemacs.

The other advantage is that we do not try to have ispell.el entries for
every language having a dict, so original alist is smaller.

> > (add-hook 'ispell-spellchecker-init-pre-hook
> >           'debian-ispell-initialize-dicts-alist)
> 
> > ``debian-aspell-only-dictionary-alist'' and
> > ``debian-ispell-only-dictionary-alist'' being built after the info
> > provided by the package maintainers.
> 
> I must say that I generally prefer if there aren't any hooks and when we
> don't use dynamic scoping, so I'm not thrilled about this code.
> I understand that there might be a need for some hook, but I'd rather it
> doesn't use dynamic scoping.  Could you get away with a normal hook run
> at the end of ispell-set-spellchecker-params, which would work by
> modifying ispell-dictionary-alist?

Disclaimer: Although I do some elisp hacking, I am far from being an elisp
expert, so do not be surprised if I need some reading before understanding
some details, or even finally do not fully understand them. Thanks in
advance for your patience.

I think the important point is at which time is this extra distro info to be
processed if present (or of course if minimal support for this possible
extra info belongs to pristine ispell.el or is something expected from
distros). I think this is better done as currently, just in the middle, that
is, after parsed aspell dicts, but before base-dicts. You do not know where
things come from if things are done at the end, and if you need to check it
you may end reusing half of the function.

If the problem is about using a hook there, something like

(when (fboundp 'ispell-distro-may-modify-dictionary-alist)
  (ispell-distro-may-modify-dictionary-alist))

will also do the work, although may look a bit ugly. If the problem is
both hooks and dynamic scoping, something like

(let ...
  (distro-fallback-dicts-alist (when (fboundp 'ispell-may-get-distro-fallback-dicts)
                                  (ispell-may-get-distro-fallback-dicts)))

in (ispell-set-spellchecker-params), where
(ispell-may-get-distro-fallback-dicts), if present, returns the alist,
may work.

Of course, is also possible to consider that this kind of distro things should
not go in pristine ispell.el.

> > Should I document the hook just in the function, or there is a better place?
> 
> Add a defvar for the hook, where you can document it.

Thanks

> >> - (setq ispell-last-program-name ispell-program-name) should be done
> >> right after checking if they're equal, not at the end of the function.
> 
> > I usually do this kind of things after the work is done, so is only changed
> > if nothing wrong happened.
> 
> In case of an error, I'd rather avoid doing it
> over-and-over-and-over-and-over ... also it's good to have it all in
> a single place.  But I won't insist.

Is not bad to insist. As a matter of fact you have convinced me in this
case. I usually prefer the other way, so errors are evident and noisy, but
here most error-possible things are condition-case protected, so there would
be no noise, just extra computing work for no good reason.

Thanks a lot for your feedback,

Regards,

-- 
Agustin