General improvement to ZSH completion + bug fix in it

[doronbehar]

EDIT: fixes #356

[0x17de]

Hey @doronbehar , some things i'd add:

at the point where you match for ^\s*path\s*=\s*: maybe use ${BASH_REMATCH[1]} later and use a capture group inside your [[ ... ]] expression to retrieve the path information - that way you'd not need to trim spaces in the end.

if some file named displayname exists inside the calendar's directory you could use the contents of that file as the resulting string.

Thanks for the changes :)

[doronbehar]

use a capture group inside your [[ ... ]] expression to retrieve the path information ...

I'm not sure what do you mean by 'capture group'. Never the less, thanks for pointing out the parameter $BASH_REMATCH! I've searched for that in the docs and so I've come up with a solution that uses features mentioned there as well. See last commit.

if some file named displayname exists inside the calendar's directory you could use the contents of that file as the resulting string.

I think this should be consulted with @WhyNotHugo because I'm not totally sure as for the behaviour of todoman in this regard. When I run e.g. todo new --list <arg>, does todoman expects the argument <arg> to be the list's 'displayname' or the list's path (relative to the directory of the whole calendars of course)?

[WhyNotHugo]

todo new --list XXX expects the displayname if one exists (since that's how the list will be referred to everywhere).

Thanks for your time on this, I'm not very fluent in zsh-completion scripts, but changes look good so far. Do you want to address the above comments in this PR too?

[doronbehar]

Thanks for commenting on that @WhyNotHugo although TBH I could have tested that myself :smile_cat:. I've tested your statement and fortunately it seems todoman does accept both arguments to --list i.e the name of the calendar as written in displayname and the name of it according to it's actual relative path on the file system.

The question is then, whether we should actually support displaying the displayname of it? I won't object that but it's kind of a hassle for me... I mean I have to read and evaluate the glob that may appear in the path given in the config and then I'll have to read the content of each of those displayname files within those directories and then group all of those options considering that not necessarily all calendars have a displayname file so these should be given as options without trying to read their displayname file...

The way things work now is with a very simple file completion function that uses the parent directory of all calendars as a prefix. I like this simplicity so I would rather not change it.

[WhyNotHugo]

Generally, displayname is what we expect people to interact with. In my case, my lists have directory names which are incomprehensible (while they have human-friendly displaynames):

502708f4-4b94-4963-b042-f7340c829bdf/
9bcef3fa-504d-4c6f-b320-fd24257de5a7/
9f57caac-df79-4488-8019-51b21648c4a8/
d6aa4f1e-bdf6-4b6b-999d-b89afd62c994/

This is because I set up vdirsyncer to pull all remote calendars, so it just uses the remote ID. I'd expect that setup to be relatively common.

[doronbehar]

Oh right then, that's a definitely good reason to implement this suggestion. I've implemented that and according to my initial testings it works well. Pushing the current version.

I'm not sure however, how should the completion handle calendar directories it finds that don't have a displayname file at all. Right now, it suggests the full paths of these. This seems reasonable in case multiple path = directives may appear and not necessarily all of them have a * which is expanded by the completion.

I hope this is not to hard to understand, I'm trying to support the use case of multiple path = directives which not all of them nescessarily have a displayname file in them - whether there's a * at the end of them or not. Is this too much? The question is: How does todoman behave when it encounters multiple path = directives with or without * in them?

[0x17de]

Hey @doronbehar , with capture group i meant the ( ) symbols inside the regex - ${BASH_REMATCH[1]} would point to the first ( ) surrounded part inside regex. E.g. ^\s*([^ ]+)\s*=\s*(.*)$ here ${BASH_REMATCH[2]} is equal to the matched data after the equals sign. To have some non capturing group you usually do something like (?: standard regex inbetween and a closing ). I would personally expect the last part of the path (basename) to be shown if no displayname is present. Thanks for including my suggestions :)

[doronbehar]

@0x17de I understood what you meant :) But according to the manual (and I tested that inside and outside the completion function), the variable BASH_REMATCH is set only if the option BASH_REMATCH is set. See here: http://zsh.sourceforge.net/Doc/Release/Conditional-Expressions.html#Conditional-Expressions

It would have been possible to set it temporarily and then unset back according to this option's state before but completion function started, similarly to what's done often with IFS. But it seems like a big hassle in my view while using $MEND achieves the same goal, as suggested right before the manual mentions BASH_REMATCH.

As for suggesting the basename in case of a lacking displayname file, I suppose you are right but I'd like @WhyNotHugo to confirm that before I'll go ahead and implement this.

Above all, I'm not sure that todoman even supports more than a single path = directives in the config, with / without * at the end of each. I hope he'll be able to enlighten us as for choosing the best way to complete these.

[WhyNotHugo]

The question is: How does todoman behave when it encounters multiple path = directives with or without * in them?

I had to check because I wasn't sure. More than one path directive is invalid, and will be rejected by todoman. So completion it's okay for completion to have "undefined behaviour" for that scenario.

[WhyNotHugo]

As for suggesting the basename in case of a lacking displayname file, I suppose you are right but I'd like @WhyNotHugo to confirm that before I'll go ahead and implement this.

Yes, that's what todoman does. :)

[doronbehar]

Oh right then, to me this is ready.

[WhyNotHugo]

Out of curiosity: would helper methods inside todoman be of any use, or is that too slow for completion scripts?

I'm thinking, of, for example, a command that prints all configuration and list settings in a shell-friendly manner.

[doronbehar]

That's an excellent suggestion! As a somewhat experienced ZSH completions writer, I have encountered completion functions which needed to be aware of the command's configuration they are completing and thus they would either need to parse the configuration file themselves or ask the program for these specific values.

Here are a few examples:

• Git: Very friendly for shell completion functions with it's git config command which prints values as parsed by Git itself.
• Taskwarrior: Has sub-commands prefixed with _ and used by shell completion functions. They complete values such as tasks' attributes and tasks' projects which are changed dynamically through out the use of the program.
• Luarocks: Has the sub command config which it's completion function uses. I happen to be the writer of it.

Todoman could certainly use such a helper command. As for it perhaps being slow, that doesn't have to be an issue if the user enables completion cache. We can base the cache validation check based on the modification date of the config file and if that's not newer then the cache, we'll check the modification date of the directory todoman reported back when we first created the cache. I've implemented a similar mechanism with Luarocks' completion.

[WhyNotHugo]

I can implement a config command. Do you think something like this is useful? Or does it end up slowing stuff? I'll fully trust your input on whether this is worthwhile or not:

$ todo config
main.path="/home/hugo/.local/share/calendars/*"
main.time_format=""
main.default_list="personal"
main.humanize="True"
main.startable="True"
main.color="auto"
main.date_format="%x"
main.dt_separator=" "
main.default_due="24"
main.cache_path="/home/hugo/.cache/todoman/cache.sqlite3"
main.default_command="list"
main.default_priority="None"
list="Holidays: Netherlands"
list="study"
list="work"
list="personal"
list="[email protected]"

Note: this took like 5 minutes, so feel free to say if it's useless. Also, format is aimed at being as shell-friendly as possible, so let me know if some other format is actually better.

[doronbehar]

As I said before, if a user has enabled completion cache, the completion function will run the todoman command only once to get the values it needs and then it'll count on a heuristic cache validation test to know if it needs an update.

Never the less, both for the sake of it being more comfortable for me and for the quickness of it for users without completion cache enabled, perhaps an option to query a specific variable only will be better. I'd print only the requested variable's value in that case.

Note that currently the completion parses dt_separator and date|time_formats as well so these will be more easily retrieved as well. They are used in todo new --due= for example. As an example for what I would expect from todo config's usage, I'd like to run: todo config main.dt_separator and todo config main.date_format and get those values.

As for the lists, I'd say that supplying main.path could be enough since the whole logic around it is already implemented but since you said this wasn't too hard, It would have been a little more comfortable to have the lists show up without the list= prefix and only when running todo config lists.

There shouldn't be a need to see all configuration variables i.e when todo config runs without an additional argument but that won't hurt of course.

[WhyNotHugo]

Sorry, it's been a while, but I don't think there's anything blocking this from being merged, right?