[system-command-line] some cleanup fixes - #4238
Conversation
vlada-shubina
commented
Dec 27, 2021
vlada-shubina
commented
- removed irrelevant TODOs
- fixed some strings that were not localized
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if (or.IsImplicit) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(parameter.DefaultValue)) | ||
| if (!string.IsNullOrWhiteSpace(parameter.DefaultValue)) |
There was a problem hiding this comment.
Should this scenario be covered with unit test?
There was a problem hiding this comment.
When writing unit tests I discovered that this code is never used at the moment: default value is set via SetDefaultValue (otherwise it won't be shown in help). This delegate will never be called when or.IsImplicit is true. Also we never read and pass the default value except for help - it is read and parsed by Edge further on.
I'm tempted to throw an unhandled exception here instead, then the logic change will be caught by existing unit tests. We can unit test this delegate, but little sense as other scenarios are covered by bigger scope tests.
Ideally this logic is moved to Edge at some point which becomes the single source of truth for template parameter conversion using Generator.ConvertParameterValueToType method and set up default/host/default if no option value for given parameter and parsing logic just need to indicate if option was specified/specified without value/specified with value and the rest is done in Edge with proper localized error handling. Part of this work is already described in #4225. Errors with invalid default values should be blocked on validation when installing.
3 participants
