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

[Stefan Monnier]

>> - ispell-spellchecker-init-pre-hook is not documented.  What are
>> distro-override-dicts-alist and distro-fallback-dicts-alist?

> The hook is intended for easier interaction of distro maintainers wrt the
> dictionary alist. E.g., in Debian ispell and aspell dictionary packages
> provide info about the dictionary that might or not be in
> original ispell-dictionary-alist. That hook is intended to allow easy
> modification of both distro-override-dicts-alist and
> distro-fallback-dicts-alist. These have the same format as
> ``ispell-dictionary-alist'' and are (very poorly) documented when
> defined in the 'let', and allow two levels of overriding,

> ``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?

> ``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

> 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?

> (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?

> 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.

>> - (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.

>> - (defvar ispell-aspell-dictionary-alist...) should be before the first
>> use of that variable.

> I just put it in the aspell stuff section, but I agree that is better as you
> propose, before (ispell-set-spellchecker-params).  I should probably move the
> ispell-set-spellchecker-params stuff below the aspell stuff, so each has an
> own block.

Either way is fine by me.

>> BTW (this is unrelated to this patch, but I just noticed it while
>> looking at the code): why are ispell-dictionary-alist* autoloaded?
>> I just tried to remove the autoloads on them and I couldn't notice any
>> negative effect.

> As you pointed out, presumably because the way menus were previously built
> required it. 

> While we are at this, since ``ispell-dictionary-alist'' is in my patch built
> from its components in (ispell-set-spellchecker-params),  its original
> definition may be simplified to just a 

> (defvar ispell-dictionary-alist nil
>   " ..... The long description of ispell-dictionary-list ...
> follows
> ...."

> probably adding a coment like

> ;; Its actual value will be set in the (ispell-set-spellchecker-params)
> ;; function

Yes, I reached the exact same conclusion.


        Stefan