Skip to content

feat: Remove the deprecated NamingConventionBinder package and migrate to System.CommandLine#760

Merged
wokket merged 4 commits into
grate-devs:mainfrom
hoangthanh28:features/thanh/Remove-NamingConventionBinder-by-SystemCommandLine
Jun 22, 2026
Merged

feat: Remove the deprecated NamingConventionBinder package and migrate to System.CommandLine#760
wokket merged 4 commits into
grate-devs:mainfrom
hoangthanh28:features/thanh/Remove-NamingConventionBinder-by-SystemCommandLine

Conversation

@hoangthanh28

Copy link
Copy Markdown
Collaborator

Related to #756

@hoangthanh28 hoangthanh28 requested a review from wokket June 17, 2026 06:52
@wokket

wokket commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

I've literally just finished a first cut at this change myself... Give me a bit to pull this branch and run some tests, I found some... oddities... in the S.CL parsing while I was working on it

@wokket

wokket commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

This is showing the same weirdness I saw in my branch earlier... try adding and running the the following test:

    [Fact]
    public void CheckParamBinding()
    {
        // I don't know if this used to work, but it's...weird... now
        var command = new MigrateCommand(null!);
        var result = command.Parse("-connString=localhost"); // oops, missed a dash
        //we'd expect an error here if we looked at result.Errors, no conn string specified
        var cfg = command.GetConfiguration(result); // and definitely not expect this to work missing the required value
        Assert.Null(cfg.ConnectionString); // and even if it was created in error it wouldn't have a value
    }

I get

Assert.Null() Failure: Value is not null
Expected: null
Actual:   "onnString=localhost"

@wokket wokket linked an issue Jun 17, 2026 that may be closed by this pull request

@wokket wokket left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick review, generally looking really good though as most of these are nits or discussion points,

// command is being constructed and when it parses, so constructing and parsing must not throw.
var act = () =>
{
var command = new MigrateCommand(null!);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't seem to throw exceptions when given an incorrect command line? It passes even if passed a commandline with dupe options (eg "-ct=10 -ct=100"). If you look at the ParseResult it has the expected error about a missing --connstring etc so maybe that's a better test?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the test case to Assert the error message instead.

Comment thread unittests/Basic_tests/CommandLineParsing/Basic_CommandLineParsing.cs Outdated
Comment thread src/grate/Commands/MigrateCommand.cs Outdated
Comment thread src/grate/Commands/MigrateCommand.cs Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, if we can clean up the legacy preconditionals that would be awesome

@hoangthanh28

Copy link
Copy Markdown
Collaborator Author

This is showing the same weirdness I saw in my branch earlier... try adding and running the the following test:

    [Fact]
    public void CheckParamBinding()
    {
        // I don't know if this used to work, but it's...weird... now
        var command = new MigrateCommand(null!);
        var result = command.Parse("-connString=localhost"); // oops, missed a dash
        //we'd expect an error here if we looked at result.Errors, no conn string specified
        var cfg = command.GetConfiguration(result); // and definitely not expect this to work missing the required value
        Assert.Null(cfg.ConnectionString); // and even if it was created in error it wouldn't have a value
    }

I get

Assert.Null() Failure: Value is not null
Expected: null
Actual:   "onnString=localhost"

System.CommandLine supports POSIX-style "attached value" syntax for single-character options: -c means -c with value . So when it sees the single-dash token -connectionString=abc, it matches the -c alias and swallows the remainder — onnectionString=abc — as the option's value.

@hoangthanh28 hoangthanh28 requested a review from wokket June 20, 2026 07:48
@hoangthanh28 hoangthanh28 marked this pull request as ready for review June 20, 2026 08:22
Comment thread src/grate/Commands/MigrateCommand.cs Outdated
@hoangthanh28 hoangthanh28 requested a review from wokket June 22, 2026 05:09

@wokket wokket left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the conflicts are resolved/tests go green 👍

@hoangthanh28 hoangthanh28 force-pushed the features/thanh/Remove-NamingConventionBinder-by-SystemCommandLine branch from ba6737a to b8b9cfd Compare June 22, 2026 10:36
@hoangthanh28

Copy link
Copy Markdown
Collaborator Author

@wokket: All checks are passed. CI green now.

Thanks.

@wokket wokket merged commit a0fe1fb into grate-devs:main Jun 22, 2026
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate away from deprecated pre-release S.CL.NamingConventionBinder

2 participants