todo plugin do not uses $TODO variable

[romulusFR]

The todo plugin

bash-it/plugins/available/todo.plugin.bash

Line 12 in 3f31891

alias t='todo.sh'

directly refers to todo.sh whereas todo aliases do not and use the $TODO env variable https://github.com/Bash-it/bash-it/blob/7c09252ac86f430b9a6eaeebfc19b20a9f6fe3ce/aliases/available/todo.txt-cli.aliases.bash. The $TODO env variable is defined in https://github.com/Bash-it/bash-it/blob/master/template/bash_profile.template.bash#L35.

My suggestion would be to use the $TODO env variable in plugin as well

[cornfeedhobo]

@romulusFR Great find! Would you be willing to submit a Pull Request to clean this up? We'll take a look if you do!

[davidpfarrell]

Greetings All,

I feel like there's some ambiguity in Bash-Its TODO support.

bash_profile.template.bash

# Set this to the command you use for todo.txt-cli
export TODO="t"

This is confusing to me and could represent any one of 3 values:

1. Reference the actual installed todo.txt-cli executable (possibly with full path)
2. The name of the alias the user has already previously configured to point to the actual todo executable
3. The name of the alias the user would like bash-it to create to point to the actual todo executable

The default value "t" suggests option 2.
The alias file suggests either option 1 or 2
The plugin file implies option 3

todo.plugin.bash

if [ -z "$TODOTXT_DEFAULT_ACTION" ]; then
  # typing 't' by itself will list current todos
  export TODOTXT_DEFAULT_ACTION=ls
fi

alias t='todo.sh'

Not sure what this script used to contain (I can see it has an edit history), but it has essentially been reduced to setting an (opinionated) alias.
A better place to set opinionated aliases would be todo.txt-cli.aliases.bash.

Moving the alias leaves the plugin with just the TODOTXT_DEFAULT_ACTION configuration.
Although useful, I don't know if its enough to warrant the existing of the plugin.

Having a look at all the supported environment variables suggests to me that there are not any other variables that could benefit from defaults:

• todo.txt-cli Environment variables

Making The Plugin Useful

Having advocated for removing the plugin, I will now see if there's some direction that could make the plugin useful.

Implied Alias

The fact that the plugin tries to create alias "t" which also happens to be the default value of the ${TODO} variable, implies to me that the plugin may have intended to name the alias based on the variable, ie. something like:

# Name of alias is defined by the user
alias ${TODO}="todo.sh"

(btw: I tested this idea locally and creating an alias named from a variable works as expected)

This has me thinking we could re-purpose the plugin to do the following:

• Set ${TODOTXT_DEFAULT_ACTION} if not already defined (existing behavior)
• If ${TODO} is already defined AND passes the _command_exists ${TODO} check, then do nothing
• If ${TODO} is not defined, but todo.sh exists, then export TODO=todo.sh
• if ${TODO} is already defined BUT fails the _command_exists ${TODO} check, then:
  • If todo.sh exists, then assign it to TODO but create an alias from the previous value:
    • alias ${TODO}=todo.sh # create alias from previous value
    • export TODO=todo.sh # modify to point to actual command
  • if todo.sh does not exist, then generate a log error message and do nothing

Essentially, this boils down to:

• Try to ensure that ${TODO} exists and is valid
• In the case where it already exists but is not valid, assume the user was indicating a desire for an alias

Side Note - Plugin Name

On a side note, the plugin would be better named as todo.txt-cli.plugin.bash since the "TODOTXT_DEFAULT_ACTION" logic is specific to that todo tool.

todo.txt-cli.aliases.bash

alias tls="$TODO ls"
alias ta="$TODO a"
alias trm="$TODO rm"
alias tdo="$TODO do"
alias tpri="$TODO pri"

Where's the t alias ?

This feels like exact place where the t alias would be defined.

Generally, our aliases get to be opinionated and non-apologetic, ie. we don't have to check if an existing command or alias exists before assigning it.

But for t I would consider an exception, since:

• The user may have already defined it (per interpretation of the profile varialble)
• The plugin may have already defined it

Why the variable?

This seems like one of the only aliases to use a user-defined variable to reference the aliased command.
In fact a quick search of the aliases folder suggests there are only a few others:

• $EDITOR
• $PAGER
• $PREVIEW
• $IRC_CLIENT

This has me wondering if the ${TODO} variable should be supported?

If we feel the variable should continue to be supported:

• The alias can't assume that the plugin is enabled
• The alias should probably abort with log error message if it is not present + _command_exists

Conclusion

My thoughts are a bit all over the place on this - I kept getting new insights as I was working through the various files.

Just the same, I think our TODO/todo.txt-cli support could use some attention.

-DF

[cornfeedhobo]

I love the above analysis. I think we should still proceed with merging the currently associated PR, just for the minor cleanup of the existing established logic, then we should have a holistic evaluation of todo.

As far as that goes, I would expect the plugin to be configuring TODO_DIR more than anything to ensure that todo installs stay identical across OSes, and that the aliases be put in a dedicated alias file. AFAIK, the history for this is because todo used to be embedded in the repo. When it was removed, this stuff just wasn't really looked at.

Edit: Also, I vote that the $TODO variable be killed as part of any holistic cleanup.

[nwinkler]

Yes, agree - the $TODO variable in the template profile file shouldn't really be there. It's been there for historical purposes, probably time to get rid of it now...

[russelltadams]

I'd like to point out that setting TODO="t" and enabling the todo plugin breaks modern-t theme, which integrates the python task list script which is called "t". The alias tls for todo.sh ls already exists, why are we creating an alias "t" at all? Was this to support some other theme? See #1374 .

Users will trip on this when they have todo.sh installed, and the todo.sh plugin enabled, and then decide to try out modern-t theme. They will at first assume that the prompt is integrated with todo.sh (aliased to "t"!!) but after a while, they will notice the count is wrong and always will be. The fix is, disable the todo.sh plugin, and install "t", then the modern-t theme will work as the author intended.

I will attempt a PR that adds a comment to the modern-t theme advising them to turn off "todo.sh plugin" and make sure "t" is installed, I think this would allow most folks transparency into the issue. Also, since I now understand how to do it, would a PR for a todo.sh integrated modern theme variant be interesting? modern-todo ?