Filter out commands before parsing requirements

[meleu]

Context: filters

Currently it's required to give a complete valid command line in order to be informed the subcommand is not available.

This MR makes the filter logic happen before parsing the requirements.

Example of the current unwanted behavior 👇

$ cat src/bashly.yml
name: cli
help: Sample application
version: 0.1.0

commands:
  - name: redis
    help: Perform actions in redis
    filters:
      - redis_running
    args:
      - name: id
        required: true
        help: ID of something

$ ./cli redis
missing required argument: ID
usage: cli redis ID

$ # 😖  I'm required to give a valid command line
$ # to be informed this subcommand is not available

$ ./cli redis 123
Redis must be running (always fails)

I want to be informed the command is invalid earlier, even with an incomplete command.

After the change in this MR the behavior is to informe the user the subcommand is not available even if the whole command line is "incomplete":

$ ./cli redis
Redis must be running (always fails)

[DannyBen]

Hmm... I see the change is minimal and the tests are passing as is (which means that if we merge this, we need to have a test that validates it).

However, I am not sure about this new order.

The command line structure is how the user communicates with the program. If an incomplete command is given, the first logical block in my mind is tell the user their communication is misunderstood. For example, with this change, you can run cli redis whatever --and whatnot and it will tell you redis is not running instead of telling you your command is wrong.

Another thing that guides the order of execution here, is that code that belongs in developer-space, usually comes after code that belongs in bashly-space. Filters are developer-space - by that I mean, imagine if the filters functionality was not present in bashly, then the developer would place it as the first thing inside the function - which would mean that the user must use a valid command before they can reach this piece of code.

And finally, I am concerned about unwanted side effects. For example, one thing that I spot is that if this change is staying, it needs to go even higher in the code, since the below bashly.yml does not behave as expected:

name: cli
help: Sample application
version: 0.1.0

commands:
- name: redis
  help: Perform actions in redis
  filters: [redis_running]
  commands:
  - name: backup
    args:
    - name: id
      required: true
      help: ID of something

  - name: restore
    args:
    - name: id
      required: true
      help: ID of something

Output the "old" order - first syntax then filter:

$ ./cli redis backup
missing required argument: ID
usage: cli redis backup ID

I need to think about it and perhaps hear more opinions for or against.

[DannyBen]

@meleu - I found another advantage for the pre-PR filter order: You have args available to you.

In fact, since the filter is last, everything is available to you, as one might expect.

Real life scenarios for it:

1. A command that needs to do something with a git branch, and the git branch is an argument to the function. The filter wants to check if the branch exists.
2. Your redis command, if a redis host / port are provided as arguments.

bashly.yml

name: cli
help: Sample application
version: 0.1.0

commands:
- name: diff
  help: Show diff with specified branch
  filters: [branch_exists]
  args:
  - name: branch
    required: true
    help: Git branch name

lib/filter_branch_exist.sh

filter_branch_exists() {
  branch=${args['branch']}
  git branch --list "$branch" | grep -q "$branch" || echo "Branch $branch does not exist"
}

Output

$ ./cli diff asd
Branch asd does not exist


$ ./cli diff master
args:
- ${args[branch]} = master

---

In fact, I was sure this hidden feature is documented in Custom Filters, but it isn't.

EDIT: Found it - in the comment of the filters example
https://github.com/DannyBen/bashly/tree/master/examples/filters#srclibfilterssh

Unless you or someone else feel strongly that the current behavior should change, I am leaning towards not merging.

[meleu]

Yeah, your points convinced me. It's better to keep the logic as is.

Closing this.