Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Support allowed argument values as an option #65

Closed
abh1kg opened this issue Mar 1, 2021 · 14 comments · Fixed by #66
Closed

[Feature] Support allowed argument values as an option #65

abh1kg opened this issue Mar 1, 2021 · 14 comments · Fixed by #66
Labels
enhancement New feature or request

Comments

@abh1kg
Copy link

abh1kg commented Mar 1, 2021

First of all, I cannot sing the praises of bashly enough - I have been using it heavily for a BASH CLI in my project for quite some time now and it just works.

One feature that I'd like to see personally is support for allowed options for arguments. For example, a suggestion for the bashly.yml could look like:

- name: login
  short: l
  help: Log on to a cool service

  args:
  - name: role
    help: Role for user
    required: true
	allowed: ["admin", "user"]

The behaviour I am expecting is:
✔️ login admin
✔️ login user
login anythingelse - this could fail with an error message like Invalid role - allowed values are "admin" and "user"

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

I am happy to hear that other people make good use of bashly, thanks for sharing that.

While I can see the value of what you are proposing, my initial reaction is that this is something that is better handled by the application space, rather than the framework space.

Why I think it should be handled by the user-domain application

Note that even in your question, you wanted to show a friendly error to the user:

Invalid role - allowed values are "admin" and "user"

So, if this kind of feature is handled by the framework, it will require doing quite a few things. Such as, handle the help message accordingly, perhaps alter the usage text, add the same kind of feature to the positional arguments, and possibly more.

Also keep in mind, one of bashly's objectives is to be easy to learn, and easy to configure. I intentionally created a minimal set of tools - so much so, that some "features" were excvluded and can be opt in by calling bashly add FEATURE.

How I propose to do it (without any change to bashly).

I think you can still implement what you want, with ease, and without "copy pasting" code around. You can even do so in a way that is easy to then copy the solution between your different scripts.

Here is what I suggest:

Step 1: Initialize a script workspace with functions lib

$ bashly init --minimal
$ bashly add lib   # this just creates a sample function in `./sec/lib`

Step 2: Update bashly.yml with a sample flag+argument

# bashly.yml
name: login
help: Sample minimal application without commands
version: 0.1.0

args:
- name: site
  help: Site to login to

flags:
- long: --user
  short: -u
  arg: NAME
  help: "User name. Allowed: user, admin"

Step 3: Create the verify_user function

# src/lib/verify_user.sh
verify_user() {
  allowed="$1"
  if [[ ! ${args[--user]} =~ $allowed ]]; then
    echo "Invalid role. Must be one of $allowed"
    exit 1
  fi
}

Step 4: Use the new user "filter" anywhere

For example, in the root command:

# src/lib/root_command.sh
verify_user "admin|user"
inspect_args

Keep in mind, I am not totally against such a feature, and while I am sure it will add value, the question is at what cost, and what does it take away in return (e.g. configuration simplicity).

How do you feel about that?

@DannyBen DannyBen added the enhancement New feature or request label Mar 1, 2021
@abh1kg
Copy link
Author

abh1kg commented Mar 1, 2021

@DannyBen : Thanks for the reply. I already make use of library scripts to make this work and it does the job just fine. Since Bashly already provides this flexibility, I am fine with not bundling this into the framework if it increases complexity.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

Ok - let's keep this open for now, so we can think about it a little.

If you have further details or suggestions on implementation (for example, error message configuration - needed or not?), please share.

@abh1kg
Copy link
Author

abh1kg commented Mar 1, 2021

A generic message template along the lines of Invalid option - allowed options are ... should be okay for me. Of course, message templating would be nice to have.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

Of course, message templating would be nice to have.

Alright. You are familiar with bashly add strings yes?
This lets you customize all of bashly's text output.

So, if we implement this feature, the message will at least be customizable in the same way - although not sure it will be able to say "Invalid role" - since this means it must be configured in the bashly.yml.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

I think we can perhaps add this functionality without any major disruption.
Will you be able to test that it fits your use case if I create a branch with it?
Are you using the Ruby gem or the docker version?

@abh1kg
Copy link
Author

abh1kg commented Mar 1, 2021

Currently, I'm using the Rubygem version but I am fine with testing with the docker version separately as well.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

Excellent. Actually its easier for both of us to test the gem directly.
I will hopefully post a branch and instructions later.

@DannyBen
Copy link
Owner

DannyBen commented Mar 1, 2021

Alright - I believe it is ready for your second opinion.

To test it (either in a new empty folder, or directly in your bashly workspace):

Create this Gemfile

# Gemfile
source "https://rubygems.org"
git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }
gem "bashly", github: 'DannyBen/bashly', branch: 'add/flag-whitelist'

Run bundle, and you are ready to test.

You can use this initial bashly.yml if you want:

# bashly.yml
name: login
help: Sample minimal application without commands
version: 0.1.0

args:
- name: site
  help: Site to login to

flags:
- long: --user
  short: -u
  arg: NAME
  allowed: [user, admin]
  # required: true
  default: user
  help: User name

You can see more details in the PR notes

@abh1kg
Copy link
Author

abh1kg commented Mar 2, 2021

Tested the change and works perfectly fine for flags 🎉 . Thanks.
Any chance if the same could be made for "args"?

@DannyBen
Copy link
Owner

DannyBen commented Mar 2, 2021

Any chance if the same could be made for "args"?

Funny. I was about to ask you if you think it is needed there as well.
I think adding it should be easy - let me take a look at it, perhaps add it to this PR.

@DannyBen
Copy link
Owner

DannyBen commented Mar 2, 2021

I merged the PR to master, and opened a new issue to track the args implementation.

@abh1kg
Copy link
Author

abh1kg commented Mar 2, 2021

Thanks a lot - I didn't check yet but was the Rubygem updated with the feature for flags already?

@DannyBen
Copy link
Owner

DannyBen commented Mar 2, 2021

Not yet - if you can wait a little longer (hours not days), I will have a release with args as well.

For now, you can use the same Gemfile, only use branch master.

I am also going to change the implementation a little (internals only) to be easier to maintain, and to add the args. Should be ready real soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants