Skip to content

Conversation

@houk-ms
Copy link
Contributor

@houk-ms houk-ms commented Dec 23, 2020

Description

This PR adds an util function to highlight a command.

The function may be used in

  • error recommendation
  • az next

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

Comment on lines 78 to 79
"""highlight a command to make it colored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some example can be very helpful, like

def resource_to_scopes(resource):
"""Convert the ADAL resource ID to MSAL scopes by appending the /.default suffix and return a list.
For example:
'https://management.core.windows.net/' -> ['https://management.core.windows.net//.default']
'https://managedhsm.azure.com' -> ['https://managedhsm.azure.com/.default']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more comments about the highlighting rules. The return type is of list of styled text, not easy to provide example.

@jiasli
Copy link
Member

jiasli commented Dec 23, 2020

The style framework is far from perfect. I made several refinements in #16258. I suggest we put this PR on hold until #16258 is fully tested and merged.

Comment on lines +93 to +97
if arg.startswith('-') and '=' not in arg:
style = Style.ACTION
argument_begins = True
elif not argument_begins and '=' not in arg:
style = Style.ACTION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add comment about the rule used?

Copy link
Member

@jiasli jiasli Dec 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some comments at #16257 (comment)

The logic of = handling is for commands with positional argument. However, it works for az config set a=b, but it doesn't work for az config unset output. It is the nature of positional argument that we can NOT decide whether an arg (like output) is

  • part of a command, or
  • a positional argument

until semantic analysis is involved.

Let's keep it simple and treat it as part of the command by now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possible solution is to utilize linter_exclusions.yml which contains the full list of commands with positional arguments:

config set:
parameters:
key_value:
rule_exclusions:
- no_positional_parameters

Copy link
Contributor Author

@houk-ms houk-ms Dec 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good proposal, but also would make the function much more complex and harder to maintain.

I prefer we keep it simple for now, until we really need to consider the case.

@jiasli jiasli mentioned this pull request Dec 25, 2020
@jiasli jiasli changed the title {Core} Add an util function to highlight a command {Core} Add util function highlight_command Jan 20, 2021
@houk-ms houk-ms merged commit bc02176 into Azure:dev Jan 20, 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.

4 participants