Fix #1629: Page command shouldn't throw on options error. #1654
Conversation
|
|
||
| if (optionValue < 0) | ||
| { | ||
| string optionDescription = CommandUtilities.GetOptionDescription<T>(optionName); |
There was a problem hiding this comment.
CommandUtilities [](start = 43, length = 16)
I have another PR out that moves this method into DriverUtilities, so whoever gets in second will have a small merge conflict to resolve. #Pending
|
|
||
| // Load the JsonMap, if previously built and up-to-date, or rebuild it | ||
| JsonMapNode root = LoadOrRebuildMap(options); | ||
|
|
||
| // Write the desired page from the Sarif file | ||
| ExtractPage(options, root); | ||
|
|
||
| return 1; | ||
| return 0; |
There was a problem hiding this comment.
0 [](start = 19, length = 1)
This was just a bug :-) #ByDesign
| @@ -50,7 +50,7 @@ internal class PageOptions | |||
| "force", | |||
| Default = true, | |||
| HelpText = "Force overwrite of output file if it exists.")] | |||
| public bool Force { get; set; } = true; | |||
There was a problem hiding this comment.
There is a bug #1630, "Multitool page command --force defaults to true and can't be changed". This change does not fix that bug!
Rather, this change makes it easier to write my new unit tests in PageCommandTests. Without this change, I had to explicitly set Force = false in every one of my test cases except for the single case where Force needed to be true.
It makes sense to remove this initialization; I don't think there's a single other property in any of the options classes that's initialized like this. It also makes sense to fix #1630 by removing the Default = true on line 51, but I don't want to do that in this PR because it's a command line behavior change and I want to validate it with you first.
Note that with Default = true, not only do I think it's the wrong default, but there's no way to specify the value "false" on the command line! There's nothing like PowerShell's -mySwitch:false syntax to turn it off. #ByDesign
|
@michaelcfanning is added to the review. #Closed |
|
@rtaket is added to the review. #Closed |
|
@harleenkohli is added to the review. #Closed |
|
@SurbhiMS is added to the review. #Closed |
ghost
requested review from
harleenkohli,
michaelcfanning,
rtaket and
SurbhiMS
|
@cfaucon is added to the review. #Closed |
There's a big comment in PageCommandTests.cs that explains the rationale for the change (the comment is there because it's also the rationale for why I arranged the tests as I did), so I won't reproduce it here. See also the description of Issue #1629.