Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate.exe could be more helpful in argument parsing error situations.. #709

Closed
untilted-document opened this issue Mar 24, 2016 · 4 comments
Labels
improvement Improvement of behavior or code quality

Comments

@untilted-document
Copy link

I'm struggling to use Migrate.exe. I wrote a command like this, following the tool's output help:

Migrate.exe -a "PathToMyMigrations.dll" -dbtype SqlServer -conn "MyNiceConnectionStringHere"

And I run it - Migrate just prints its arguments again and exits with code 1. What was wrong? Why didn't it tell me what it didn't like? Here's a go with definitely broken args (fake assembly and no "conn"):

image

A message like "--conn is required" or "Assembly 'a' could not be loaded" would be good. That's if conn is indeed required - wiki says it is, console help is inconclusive

I actually cribbed my first stab from a blog:

Migrate.exe --assembly="PathToMyMigrations.dll" --dbtype=SqlServer --conn="MyNiceConnectionStringHere"

Which led me to have to read a lengthy wiki article on the arguments format, and why one example looked completely different to the other. I'm now educated (not that it helps solve my original problem) but feel the way you do your arguments help text is a little confusing. I appreciate that you're trying to offer maximum flexibility with them but this just reads like the author is deranged:

"If a parameter has an = sign after it, then it expects some text value to be set ... [and] ... the = sign is optional ... Single dashes or double dashes is also optional
--verbose=VALUE ... This should be a boolean switch but is not. VALUE can be anything to set to true and the switch should not be specified to set it to false"

So --verbose=false is verbose? What if I say -verbose? Is it true or not, because it's present but doesnt have a value. Erm.

Examples that present --conn and -c like the number of hyphens is significant are also misleading? When a super-duper easy-to-use-because-it-can-make-sense-of-any-old-garbage-passed-in algorithm starts needing a convoluted explanation, it's not really achieving its design goal any more

Do you think, for the sake of consistency, you could please settle on one format, make all your help texts show examples in that format and then discreetly stop talking about (even if you still support for legacy reasons) the argument formats of having space/equals or not, values or not.. (And also tell me what it doesn't like about the arguments I'm passing)

@untilted-document
Copy link
Author

Ah.. I've just worked it out. I was passing "dbtype" (just like the wiki says I can) and it was barfing (silently) - quick change that t -> T and it all starts working.

So.. The arguments parser bends over backwards to make it so I can sling any old junk in there, in any order, with or without this or that.. but it's CASE SENSITIVE?

:|

@abusby abusby added the improvement Improvement of behavior or code quality label Mar 30, 2016
@abusby
Copy link
Contributor

abusby commented Mar 30, 2016

They really should be case insensitive on a windows command line.

@RobPethick
Copy link

Would I be right in saying there are a couple of issues here

  1. Better error messages explaining why the command failed/which argument is missing
  2. Case insensitive option names
  3. Whether to move to being more opinionated about what format the option names should take.

I'd be happy to work up PRs for 1/2

@untilted-document
Copy link
Author

I took a look at the code of MigratorConsole.cs and it seems one problem is:

                if (string.IsNullOrEmpty(ProcessorType) ||
                    string.IsNullOrEmpty(TargetAssembly))
                {
                    DisplayHelp(optionSet);
                    Environment.ExitCode = 1;
                    return;
                }

Because the opt are case sens, dbtype isnt picked up, so this IF is true, but the code handling the case doesn't really go very far in helping the user out. It'd maybe help to expand it to two ifs, one for each case, and include a message "Expects 'x' which was not supplied, remember options are case sensitive" or similar relevant message depending on any rework to make opts case insens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of behavior or code quality
Projects
None yet
Development

No branches or pull requests

4 participants