Fix broken command line options

[gathogojr]

This pull request fixes #390, fixes #391, fixes #384, and closes #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 #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: commandlineparser/commandline#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

• Added tests for the CLI command options and code generation
• Add support for ServiceName command line option to the CLI - This makes it possible to control the CSDL filename (Created as "Service NameCsdl.xml" by default)

[gathogojr]

@gregwinterstein FYI

[gathogojr]

Changes to this solution file are due to reorganizing the solution through introduction of src and test solution folders and moving different projects to their logical solution folder

[habbes]

Why do we check string.IsNullOrEmpty if the wrapping if-block already checks that the string is null or all whitespace (which includes empty).

[habbes]

Also, does this mean that with your changes that the namespacePrefix = generateOptions.NamespacePrefix condition does not get reached anymore?

[habbes]

I have the same question for all the similar changes to the code blocks below.

[gathogojr]

@habbes Like I explained offline, when either of namespace-prefix, excluded-operation-imports, excluded-bound-operations, and excluded-schema-types is explicitly specified from the command line as an empty string, that should interpreted as an indication that if there was a non-empty value specified previously undone, it should be undone. Otherwise, one would never be able to undo any namespace prefix, or excluded model elements from the config file.
As an illustration, consider a scenario, where on the first occasion I elected to have a namespace prefix "Client":

generate --metadata-uri http://tempuri.org --outputdir C:\tempdir --namespace-prefix Client

You end up with a config file with the following properties (among others):

{
    "Endpoint": "http://tempuri.org",
    "UseNamespacePrefix": true,
    "NamespacePrefix": "Client"
}

The logical way to unset the Client namespace prefix from the config file is as follows:

generate --metadata-uri http://tempuri.org --outputdir C:\tempdir --namespace-prefix "" --config-file C:\tempdir\ConnectedService.json

Where the empty string will be interpreted to mean I want to overwrite the value in the config file.

Same case with excluded model elements.
Consider a scenario, where on the first occasion I elected to exclude "ResetDataSource" operation import:

generate --metadata-uri http://tempuri.org --outputdir C:\tempdir --excluded-operation-imports ResetDataSource

You end up with a config file with the following properties (among others):

{
    "Endpoint": "http://tempuri.org",
    "ExcludedOperationImports": ["ResetDataSource"]
}

The logical way to unset the ResetDataSource excluded operation import from the config file is as follows:

generate --metadata-uri http://tempuri.org --outputdir C:\tempdir --excluded-operation-imports "" --config-file C:\tempdir\ConnectedService.json

[habbes]

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

[gathogojr]

@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

[ElizabethOkerio]

Up there. you state that if namespace prefix is null then no namespace prefix should be applied. Why are we applying the namespace prefix here? or What does configUserSettings?.NamespacePrefix contain

[gathogojr]

@ElizabethOkerio My statement is different - "if namespace prefix is empty ...".
The namespace prefix will be null if namespace-prefix is not included in the command - in which case it should be okay to apply whatever is in the config file, and will empty if it's included in the command with an empty string - in which case we shouldn't apply whatever might be in the config file (and we'll save the updated config file with empty namespace prefix).

[habbes]

I think the reasoning here is that the namespacePrefix could either be set through command option or config file. If it's not set by the command option, we check the config file, if it's no set there either, it would be null. But also, I believe @gathogojr is making a clear distinction between empty and null. If the namespacePrefix is an empty string, then the user has explicitly set it to an empty value, which would remove the current prefix if it exists.