SimonCahill/getopt.net

It is not possible to put a bare filename on the commandline

Closed this issue · 8 comments

Bare filenames (e.g. less LICENSE) are permitted by getopt, but don't work with this project. I don't know if you're aiming for POSIX parsing, GNU parsing, or both, but at least four of these six tests should pass.

[TestMethod]
public void TestFilenameOnly() {
    ShortOpts = string.Empty;
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt" };
    GetNextOpt(out var _); // Something expressing the existence of filename.txt should happen here.
}

[TestMethod]
public void TestFilenameWithPreceedingDashes() {
    ShortOpts = string.Empty;
    Options = Array.Empty<Option>();
    DoubleDashStopsParsing = true;
    AppArgs = new[] { "--", "--filename.txt" };
    GetNextOpt(out var _); // Something expressing the existence of --filename.txt should happen here.
}

[TestMethod]
public void TestOptionBeforeFilename() {
    ShortOpts = "t";
    Options = Array.Empty<Option>();
    AppArgs = new[] { "-t", "filename.txt" };
    var optChar = (char)GetNextOpt(out var optArg);
    Assert.AreEqual('t', optChar);
    Assert.IsNull(optArg);
    GetNextOpt(out var _); // Something expressing the existence of filename.txt should happen here.
}

[TestMethod]
public void TestFilenameBeforeOptionGnuParsing() {
    ShortOpts = "t";
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt", "-t" };
    var optChar = (char)GetNextOpt(out var optArg);
    Assert.AreEqual('t', optChar);
    Assert.IsNull(optArg);
    GetNextOpt(out var _); // Something expressing the existence of filename.txt should happen here.
}

[TestMethod]
public void TestFilenameBeforeOptionGnuInOrderParsing() {
    ShortOpts = "-t";
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt", "-t" };
    var optChar = (char)GetNextOpt(out var optArg);
    Assert.AreEqual('\x01', optChar);
    Assert.IsNull("filename.txt");
    optChar = (char)GetNextOpt(out optArg);
    Assert.AreEqual('t', optChar);
    Assert.IsNull(optArg);
}

[TestMethod]
public void TestFilenameBeforeOptionPosixParsing() {
    ShortOpts = "t";
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt", "-t" };
    GetNextOpt(out var _); // Something expressing the existence of filename.txt should happen here.
    GetNextOpt(out var _); // Something expressing the existence of -t, as if it were a filename, should happen here.
}

I'll check it out and see why filenames aren't parsing. Give me some time to test.

Thanks for your input!

In your test TestFilenameWithPreceedingDashes, where your test code is

[TestMethod]
public void TestFilenameWithPreceedingDashes() {
	ShortOpts = string.Empty;
	Options = Array.Empty<Option>();
	DoubleDashStopsParsing = true;
	AppArgs = new[] { "--", "--filename.txt" };
	GetNextOpt(out var _); // Something expressing the existence of --filename.txt should happen here.
}

GetNextOpt is behaving as desired.

You have not set any ShortOpts or Options, DoubleDashStopsParsing was set to true, and from the man page:

The special
argument "--" forces an end of option-scanning regardless of the
scanning mode.

Unless I've misunderstood something. In that case please let me know.

I have the feeling this whole thing warrants a wiki :)

For now this issue has been fixed, I'll close if after I've merged the pull request.

"Forces an end of option-scanning" means that everything remaining is a filename-like argument.

Both TestFilenameOnly and TestFilenameWithPreceedingDashes throw ParseException("Unexpected option argument!"). GetOptNext should provide some graceful message (maybe by returning -1?) that the remaining arguments need to be processed by the caller as if they were filenames (or grep patterns or sed expressions), regardless of whether or not they start with hyphens.

"Forces an end of option-scanning" means that everything remaining is a filename-like argument.

Ah, I see what you mean. That's where I misunderstood it. Fair enough, I'll get to adding that.

Both TestFilenameOnly and TestFilenameWithPreceedingDashes throw ParseException("Unexpected option argument!").

Yes, this is intended behavior but can easily be disabled by setting IgnoreInvalidOptions to true. But I feel this option should be default true to prevent further confusion.

Here are the tests which pass and show expected behavior:

[TestMethod]
[ExpectedException(typeof(ParseException))]
public void TestFilenameOnly_ExpectException() {
    ShortOpts = string.Empty;
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt" };
    GetNextOpt(out var _);
}

[TestMethod]
public void TestFilenameOnly_IgnoreInvalidOpts() {
    ShortOpts = string.Empty;
    Options = Array.Empty<Option>();
    AppArgs = new[] { "filename.txt" };
    IgnoreInvalidOptions = true;
    string optArg;
    Assert.AreEqual(InvalidOptChar, (char)GetNextOpt(out optArg));
}

In your opinion, would this be the correct implementation?

public bool StopParsingOptions { get; set; } = false;

public int GetNextOpt(out string? outOptArg) {
    // ...
    else if (DoubleDashStopsParsing && AppArgs[CurrentIndex].Equals(DoubleDash, 
    StringComparison.InvariantCultureIgnoreCase)) {
        m_currentIndex++;
        StopParsingOptions = true;
        return GetNextOpt(out outOptArg);
    }

    // Check here if StopParsingOptions is true;
    // if so, then simply return NonOptChar and set outOptArg to the value of the argument
    if (StopParsingOptions) {
        outOptArg = AppArgs[CurrentIndex];
        m_currentIndex++;
        return NonOptChar;
    }

    // ...
}

Without writing code to test this, I'd say that a filename is not an option. If I say ShortOpts = "t", I want to be able to tell the difference between myapp -te (incorrect; -e is not allowed) and myapp -t e (correct; e is probably a filename).

That implementation looks correct, as far as I can see it. I'd've put the if (StopParsingOptions) block at the beginning of the method so I know it'll get the first chance to interrupt parsing, but that's probably just a style difference.

Without writing code to test this, I'd say that a filename is not an option

I agree 100%, hence the previous failures.

If I say ShortOpts = "t", I want to be able to tell the difference between myapp -te (incorrect; -e is not allowed) and myapp -t e (correct; e is probably a filename).

This is already handled correctly by getopt.net. In this case, only t will be returned, and e is considered a different option. Only during the next execution of GetNextOpt would e be discovered to be an error.