-
Notifications
You must be signed in to change notification settings - Fork 379
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
[system-command-line] instantiate logic #4086
[system-command-line] instantiate logic #4086
Conversation
subject to discussion whether we need this logic or not
// Reporter.Error.WriteLine(string.Format(LocalizableStrings.MissingRequiredParameter, fixedMessage, resultTemplateName).Bold().Red()); | ||
// } | ||
// return New3CommandStatus.MissingMandatoryParam; | ||
//this is unlikely case as these errors are caught on parse level now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "unlikely" means here? Never(should we throw exception)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously we did validation only for choice parameters on CLI level and didn't check if mandatory parameters are specified. So we end up in this code pretty often.
Now we do validation for all kinds of parameters on the parser level. So if we do job right, this should not happen. But in case logic of validation diverges in parser and core we might end up here.
Imo it's best to display Edge error here as we don't know the reason. Possible improvement is to separate invalid parameters and reason in Edge. Then we can do display reason, and convert canonical (name in json) parameter name to alias not to confuse user with canonical name. Edge doesn't know the alias name (this is CLI specific).
Reporter.Error.WriteLine($"Template {args.ShortName} doesn't exist."); | ||
return NewCommandStatus.NotFound; | ||
} | ||
//TODO: decide what to do if there are more than 1 group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we dicussed this, if 2 groups have same ShortName, for both groups instead of shortname we display GroupdId.
…plemenation with host data
87cc3c9
to
4b5483d
Compare
4b5483d
to
d7eef97
Compare
Problem
#3809
Outstanding (to be solved in different PR):
--allow-scripts
only if the template has run-script post actionOutstanding (likely to be done in this PR)
Solution
basic solution for instantiate template (including basic help).
Checks:
#nullable enable
to all the modified files ?