Allow empty string as flag argument

[wolfgang42]

(This overlaps with the changes in #113, but came up in an unrelated context and seemed worthy of discussion on its own merits.)

I was explicitly passing in the empty string as an argument to a flag, but bashly treated this as though the argument was absent, which is not the behavior I wanted. This PR changes that so that it will accept the empty string correctly, and adds a spec using the example of a blank password.

I think the same change should also be made to non-flag arguments, though I haven't investigated that case yet.

[DannyBen]

Why is this needed?

[wolfgang42]

Why is this needed?

I tried to explain in the PR description, is there something that isn't clear?

[DannyBen]

I tried to explain in the PR description, is there something that isn't clear?

Yes. Why is it needed to allow a parameter to receive an empty string? What is the use case that this solves? If password can be empty, don't make it a required flag. If it cannot be empty, make it a required param.

To add to this - I never once had to use --flag '' in any command line utility. This is just bad form to allow it.

$ git commit -am ''
Aborting commit due to empty commit message.

$ docker build -t '' .
invalid argument "" for "-t, --tag" flag: invalid reference format
See 'docker build --help'.

Everywhere I look, this is an "abort" condition.
This, is good form. Empry string == Empty and unset for the purpose of command line arguments.

Although, this is slightly different than what Bashly is doing, it is bashly's responsibility to ensure that once it reaches the user code, everything is valid. This is considered invalid in my mind.

[wolfgang42]

It solves the use case of passing an empty string as a parameter. This is very distinct from not passing the parameter at all. Some random examples I found in the oilshell corpus:

git rev-parse --prefix "" -- top sub1/file1
whiptail --title "$WHIP_TITLE" --backtitle "$WHIP_BACKTITLE" --menu "" --default-item "$MENU_MAIN_LASTITEM" --cancel-button "Exit"
adduser --disabled-password --gecos '' mesos
ssh-keygen -q -N '' -t ed25519 -f $keyfile
cut -d '' -f1
pathchk -P ''
git commit -m "" --allow-empty-message
git tag -m "" tag-name
ldapsearch -R -T -h "$dc" $ldap_args -b "" -s base ""

It is very surprising to me as a script author that bashly is attempting to perform validation on my script's arguments without any context as to what they're used for. If an empty string is not part of my script's domain, I can check that case along with other validations on the value; but when it is part of the valid domain, I need Bashly to let it through in order for my script to work properly.

It's also worth noting that the examples you provide are both clearly explicit validations: the error is not "you are missing an argument" but "you have passed an argument, but its value is not valid in this specific context".

[DannyBen]

All are either niche examples, or bad design in my mind.
Why do you need --allow-empty-message instead of just making -m MESSAGE optional?

The only use case that might be relevant is for scripts that operate on strings, where clearly '' means something.

I feel that for 90% of cases, --arg '' is better blocked than allowed to pass.
And most of the other 10% can be solved with better design.

I am not yet convinced that this PR makes bashly better.

[wolfgang42]

All are either niche examples,

Niche they may be, but that is not the same as useless.

or bad design in my mind.

Personally, I think the design is perfectly good, though that is of course a matter of taste.

Why do you need --allow-empty-message instead of just making -m MESSAGE optional?

Because -m '' is not the same as leaving -m off, and --allow-empty message is orthogonal. The way git commit works is that if none of --message, --file, --reuse-message, or --no-edit are passed, it opens a text editor. Separately, if --allow-empty-message is passed, the combined result of these flags (and the subsequent EDITOR invocation) is allowed to be empty; but this flag is not especially relevant for the purposes of this discussion.

If git did what you propose (allowing empty messages unconditionally, and defaulting to an empty message if -m was not passed), then git commit on its own would by default create every commit with no message, which would be a very unhelpful way for it to work. By passing -m '' the user is indicating “I know that this is unusual, but I really do want a blank message and no text editor”.

The only use case that might be relevant is for scripts that operate on strings, where clearly '' means something.

All scripts operate on strings; I assume here that you mean “strings which do not have a deeper semantic meaning, like file paths or docker tags.” It is of course correct that there are certain types of string values for which the empty string is not a valid value (and that this is a very common validation), but (a) many of these string types also have other validations anyway and (b) it is equally true that there are many string types for which this is not the case.

I feel that for 90% of cases, --arg '' is better blocked than allowed to pass.

10% is quite a lot of cases to make impossible.

I am not yet convinced that this PR makes bashly better.

Well, hopefully I can convince you :-)

As I mentioned, this is actually some of the same changes as in #113 which you have indicated approval of, but when I noticed I needed this empty string case in my script I thought it would be useful to raise it as a separate discussion.

[DannyBen]

Lets simplify the argument.

From the bashly manifesto:

Bashly is responsible for ... Preventing your script from running unless the command line is valid.

More often than not, for most users, with most flags - an empty string is an invalid input.
Making this change, will force people to have to write custom error handling in every function they have. This is unacceptable.

Although I am yet to see a use case that does not have a better solution, I can accept that it is a matter of style.

How about enabling these empty params conditioanlly, as follows:

• Allowing both arg params and positional params to accept empty string (using the syntax proposed in this PR)
• Adding a check after the param was captured, and aborting if it is empty with a new text message that is more appropriate.
• This abort condition will be allowed to pass if the arg has an allow_empty: true in their config

I might also be open to the idea that the default behavior should be allow_empty: true.

How do you feel about that?

[DannyBen]

@wolfgang42 - since I much rather work on small PRs, dealing with small concepts, how do you feel about the following:

1. We move forward with your view point, and set this as the default (and only) behavior at this time.
2. Before we do - we also implement this in the parsing of positional arguments, if possible - since it is my understanding they are no different in this regard.
3. We remove the extra line in the test you have implemented here - and instead, we do a dedicated test (not example) in the fixtures directory, for both empty arg param and empty positional param.
4. We merge as is
5. In the future - if and when I feel that this new default is not in line with bashly's aim to only pass valid arguments to your code, I will update it myself and possibly add this allow_empty feature (which I am not yet 100% sure about).

Deal?

[wolfgang42]

I was actually thinking more about this last night and was going to suggest allow_empty as a possibility. (Actually, I had the idea for a more general validations system, which I have written down in #125 for discussion).

Your plan sounds good, except—I am not sure about making empty strings allowed and then later adding the allow_empty option again. It seems confusing for script writers to have this validation go away and come back again some time later, though I suppose since bashly is still v0.x this is maybe to be expected.

The extra test line was mainly to demonstrate the point, for discussion; I can definitely make a proper fixture test for both flags and arguments, if we decide that this PR is a good approach.

Incidentally, I just want to say: I have sometimes taken a somewhat combative tone in these discussions, because I have quite strong opinions about how some of these things should work, but I appreciate the way that you also have similarly strong opinions and a cohesive vision for how Bashly should work. I feel that, though it may take a while to get there, the final result will be the stronger for it.

[DannyBen]

I am not sure about making empty strings allowed and then later adding the allow_empty option again

I was actually moving closer to your original opinion, meaning I suggested to implement this as a new default, and later, if at all, add allow_empty: false if someone wanted to disable it for a given flag.

However - if you do not feel strongly that you need this empty feature, I would leave it alone for now.

In addition, I saw #125 and I love it! I feel this is definitely the way to go.

... though I suppose since bashly is still v0.x this is maybe to be expected.

Although this is not yet 1.x, I still prefer not to break compatibility.

Incidentally, I just want to say...

This is perfectly fine, I haven't noticed... I feel we both have strong opinions, loosely held. Given new facts, I am happy to change my mind.

However, I have learned that there is no way to make everybody happy, and the only way to keep a product solid, is to defend its values - otherwise, it just becomes a mix of different opinions and patchy features.

but I appreciate the way that you also have similarly strong opinions

Likewise. I definitely see the value in these discussions, and your arguments are always well outlined, so it gives me another perspective about how bashly is used by someone other than myself.

In light of #125 - should we close this one for now, or do you prefer to implement this "allow empty as a default behavior" before we do?

[wolfgang42]

implement this as a new default, and later, if at all, add allow_empty: false if someone wanted to disable it for a given flag.
Although this is not yet 1.x, I still prefer not to break compatibility.

Ah, that sounds much less confusing.

However - if you do not feel strongly that you need this empty feature, I would leave it alone for now.

I do need empty argument support for my use case; right now I'm using a fork with this PR in it.

In light of #125

As you said, it is better to work with small PRs. I think that this is a prerequisite for adding validation anyway (at least, validate: not_empty will not work without this PR, since right now it fails before the validations would run).

I take some time soon to clean this one up per your plan #122 (comment) above so it can be merged.

[DannyBen]

I take some time soon to clean this one up per your plan #122 (comment) above so it can be merged.

Sounds great.

[cjbrigato]

The empty argument is, in niche case, the only way to respect POSIX guidelines on utility argument syntaxes, guideline that shaped much of our aquired tastes up to now.

Guideline 7:
Option-arguments should not be optional.

An empty argument permit fullfiling this rule. Later validation or de-facto broken usage is up to deal with it.

[wolfgang42]

@DannyBen I've created a fixture test and removed the example test I had added. It turns out that positional arguments already worked, but I have added a test case for them anyway. I believe this PR is ready for your review.

[DannyBen]

Hmm - don't know why GitHub didn't notify me on this - just saw it now.
What about the conflicting files?

[DannyBen]

Had one comment, and there is the issue of the conflicted files,

[DannyBen]

Shouldn't we add another arg, so that we have one optional and one required? As I have learned in #130, there are two places these args are handled.

[DannyBen]

@wolfgang42 - I can confirm this is not working as expected with the below YAML:

name: cli
help: Sample application
version: 0.1.0

commands:
- name: test
  help: Run test
  flags:
  - long: --flag
    arg: value
  args:
  - name: required_arg
    required: true
  - name: optional_arg

I think this YAML should be used in the fixture test, with these test commands:

./cli test a b --flag c
./cli test ''
./cli test '' --flag ''
./cli test '' '' --flag ''

(I played with it a little in #135).

[DannyBen]

I am closing this, as it is superseded by #135.