Re: PATCH: completion

["Bart Schaefer" <schaefer@xxxxxxxxxxxxxxxxxxxxxxx>]

On Oct 26,  1:03pm, Sven Wischnowsky wrote:
} Subject: PATCH: completion
}
} The code only needed by compctl is moved into the `compctl'
} module. The new completion stuff has its own module `complete'

Sounds good.

} the `comp1' module became almost empty so I just removed it entirely 
} and made `complete' depend on `zle'

Fine, I think.

} `complete' implements the basic stuff for completion, I made
} `compctl' depend on it

Also fine.

} Note also that I made the `complete' and `compctl' module be
} automatically autoloaded

Which also means "compiled in to static builds".  Did you try a static
build?  I guess I'll find out soon enough.

} Fine, the next thing was to remove `compgen'. This is `replaced' (made, 
} hopefully, superfluous) by adding more special parameters to the
} `parameter' module. Some of them are probably not nice or should be
} done in a completely different way. Please be so kind to look at them

Will do.

} - Since this was asked by Bart: yes, I made `functions' show disabled
}   functions, too. I think this is right because `functions' should
}   show the contents of the functions hash table.

I don't think I agree with this.  The purpose of "functions" is not just
to dump the hash table; it's for user information.  If a function has
been disabled for some reason, it shouldn't be shown:  "disable" is among
other things a sysadmin tool for concealing things that certain users
should not see.

Which, incidentally, makes me think of something I hadn't before:  If a
sysadmin wanted to prevent users from changing the completion setup in
the old system, he could simply "disable compctl" in /etc/zshenv.  Now,
there's no way to lock in a fixed set of completion functions.

Do we need to be able to mark functions read-only, as can be done with
parameters?

Anyway, it's the job of "disable -f" (with no args) to list the disabled
functions, so that if you "disable disable" there's no way to see them at
all.  Probably "disable +f foo" should list foo if it's disabled; right
now it tries to disable the builtins "+f" and "foo".

}   However, the way they 
}   are currently represented (with the `<disabled>' prefix) is not nice 
}   and I'd like to hear if anyone can think of something better.

"Not nice" doesn't even begin to cover it:  `eval "$(functions myfunc)"'
might read from a file named "disabled" and write it to "myfunc"!

}   Maybe we should just add a `disabled' parameter containing all
}   disabled functions, aliases, etc.

Can't do that without disambiguating somehow -- you could have an alias,
a function and a command all named "foo" all of which are disabled.
However, this is probably the best way to go.

} - For the `userdirs' parameter I move the fast YP/NIS code into
}   `hashtable.c' (where the fill-table-function used `getpwent()').

There was already too much stuff in hashtable.c, IMO ... it'd be nice
to have one file that is the hash table implementation, and one or more
others that create each of the hash table instances.

} - The `nameddirs' parameter is probably a bit weird, because it
}   doesn't look at parameters. So if you have `foo=/tmp', `~foo' will
}   work, but there is no `$nameddirs[foo]' entry. On the other hand,
}   this may be the right thing (and `unhash' does the same).

Yes, I think this is the right thing.  If the completion function wants
to include variables as well, it can search for them itself.  The special
param shouldn't make it appear that things have been added to the namedir
table before they really have.

} - `historywords' reports all words from the history.

Good golly.  That sounds like a great way to eat a lot of memory ...

} To access the zle widgets and keymap names I added the `zleparameter'
} module.

Sounds OK.

} You'll note that both parameters defined by `zleparameter' have the
} `zle' prefix. The difference to `parameter' is intentional to make
} sure that I will some day change either of them. So, for the last
} time, I'll ask: should we rename all parameters from the `parameter'
} module to `z*'? More precisely: is anyone against that change? Is it
} too late?

I don't think it's "too late," but I don't have a strong opinion either
way on whether to add a prefix.  Given that they're all now "hidden" by
default, so that they won't conflict with locals declared in e.g. ksh
functions, I don't think it's as big a deal, and the consistency with
e.g. $signals (for which it _is_ too late) is nice.

On the other hand, reducing collisions is probably a good thing too.
 
} If you load the parameter module in your `.zshrc', you'll notice that
} `compinit' now loads `parameter' and `zleparameter'. Without `compgen' 
} we need it in too many places to get away without it.  And now the same 
} question as for the `computil' module: should we make them be
} automatically autoloaded (and linked in for static shells)

Yes!  If the stock set of completion functions are useless without them,
they have to be linked into static shells.  However, it would be nice if
they weren't actually "loaded" in the zmodload sense until they were
needed, even in a static shell.  That's going to require some changes to
the way zmodload and static modules work.

More when I get a chance to apply this (which may not be until next week).

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com