Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Feb 17, 2020

Original PR Azure/azure-cli#12234

This PR:

  1. Move yaml and yamlc output from Azure CLI to knack to unify output formats into one place
  2. Add Unicode support in yaml output, like こんにちは
  3. If stdout is a not tty, override jsonc with json, yamlc with yaml to remove color

@jiasli jiasli requested review from jsntcy and mmyyrroonn February 20, 2020 11:19
knack/output.py Outdated

def get_formatter(self, format_type): # pylint: disable=no-self-use
if not sys.stdout.isatty() and format_type in ['jsonc', 'yamlc']:
format_type = format_type[:-1]

Choose a reason for hiding this comment

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

format_type = format_type[:-1] [](start = 12, length = 30)

it works when you using string slice method to convert jsonc to json, but the code seems not easy to read, and if there is new type need convert like cjson->json, this code is not reusable. Can you explicitly map jsonc to json in your logic? Besides, I notice you add jsonc,ymalc in the OutputProducer._FORMAT_DICT above, what do you return different formatter here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the suggestions. Refactored as requested. Also added test for removing color.

@jiasli jiasli requested a review from qianwens February 26, 2020 04:11
Copy link

@mmyyrroonn mmyyrroonn left a comment

Choose a reason for hiding this comment

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

Just one question. Even if this PR is merged, this won't work until you remove the inherited logic in azure-cli-core. Right? I mean this get_formatter part.

@jiasli
Copy link
Member Author

jiasli commented Feb 26, 2020

Just one question. Even if this PR is merged, this won't work until you remove the inherited logic in azure-cli-core. Right? I mean this get_formatter part.

yes

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.

3 participants