OData / ODataConnectedService

A Visual Studio extension for generating client code for OData Services
Other
79 stars 41 forks source link

Fix broken command line options #392

Closed gathogojr closed 3 months ago

gathogojr commented 4 months ago

This pull request fixes #390, fixes #391, fixes #384, and closes https://github.com/OData/ODataConnectedService/pull/389

This appears to have broken when introducing a feature that allows loading the command line options from a config file. Here's the pull request https://github.com/OData/ODataConnectedService/pull/363

It's a useful feature since it helps someone avoid having to specify the command line options all the time.

For the feature to work in a manner that makes logical sense, the value of a command line option that is explicitly specified should take precedence over the value loaded from the config file.

There was one challenge with this though...

Let's say the value for a particular option is true in the config file, how would we differentiate between false that is explicitly specified from the command line (such that it should take precedence over the config file true option) if an unspecified option also defaults to false? This is the reason we changed the boolean options to be nullable such that their values would be null when not specified.

Users should still to be able to specify boolean options without an explicit true, e.g., --enable-tracking as opposed to --enable-tracking true. However, by making the options nullable, that capability broke. When a boolean option is not explicitly set to true or false, it's defaulting to null.

The workaround currently is to explicitly specify true or false, e.g., --enable-tracking true or --enable-tracking false.

I have since discovered that we could have achieved the objective without making the options nullable from the GenerateCommand class. We only need to make the property nullable from the GenerateOptions, i.e.:

public class GenerateCommand : Command
{
    public GenerateCommand()
        : base("generate", "...")
    {
        // ... other options
        Option enableTracking = new Option<bool>(new[] { "--enable-tracking", "-et" })
        {
            Name = "enable-tracking",
            Description = "Enable entity and property tracking."
        }
        // ... other options
    }
}

public class GenerateOptions
{
    // ... other properties
    public bool? EnableTracking { get; set; }
    // ... other properties
}

This resolves the issue. For example, with the above change, both --enable-tracking and --enable-tracking true initialize the EnableTracking property to true, while unspecified options default to null.

From my investigation, I also discovered that other users of the System.CommandLine library had made a feature request for command line options to support three states - undefined|null/true/false: https://github.com/commandlineparser/commandline/issues/316 That feature has already been implemented and upgrading the CLI project to the latest version of the System.CommandLine library (without making any change) would also solve the issue.

This pull request resolves the reported issue by removing the ? operator from the options in GenerateCommand class. It also upgrades the System.CommandLine library to the latest version.

Additional work

gathogojr commented 4 months ago

@gregwinterstein FYI

habbes commented 4 months ago

@gathogojr thanks for adding the tests. Do they run in the build pipeline as well?

gathogojr commented 4 months ago

@gathogojr thanks for adding the tests. Do they run in the build pipeline as well?

@habbes This was a good catch. I updated the build pipeline to run the CLI tests