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

[system-command-line] instantiate logic #4086

Merged

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented Nov 3, 2021

Problem

#3809

Outstanding (to be solved in different PR):

  • ambiguous template group logic
  • ambiguous template logic
  • error handing for invalid parameters (it needs to take into account possible combinations from other template groups)
  • tab completion
  • error handling for alias assignment
  • parsing hex params (low prio)
  • invalid template logic (duplicate parameters etc)
  • invalid parameters on exec time (low prio, unlikely to happen)
  • show --allow-scripts only if the template has run-script post action

Outstanding (likely to be done in this PR)

  • unit tests

Solution

basic solution for instantiate template (including basic help).

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

// 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
Copy link
Contributor

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)?

Copy link
Member Author

@vlada-shubina vlada-shubina Nov 4, 2021

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.
Copy link
Contributor

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.

@vlada-shubina vlada-shubina marked this pull request as ready for review November 10, 2021 11:04
@vlada-shubina vlada-shubina requested a review from a team as a code owner November 10, 2021 11:04
@vlada-shubina vlada-shubina merged commit c12ef77 into dotnet:feature/system-command-line Nov 10, 2021
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.

2 participants