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

[Event] Add EVENT_CLI_SUCCESSFUL_EXECUTE #224

Merged
merged 3 commits into from
Dec 16, 2020
Merged

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Dec 7, 2020

Context

In the current implementation, the command handler is invoked at L215:

cmd_result = self.invocation.execute(args)

The JSON is printed at L221:

self.output.out(cmd_result, formatter=formatter, out_file=out_file)

Therefore, anything printed by the command handler will be shown before the JSON output.

Change

Add a new event EVENT_CLI_SUCCESSFUL_EXECUTE. It is raised after a command is successfully executed, allowing a callback func to be registered and called after JSON is printed.

This enables post-output hint to be displayed after the JSON output.

knack/cli.py Outdated
@@ -219,6 +219,7 @@ def invoke(self, args, initial_invocation_data=None, out_file=None):
if cmd_result and cmd_result.result is not None:
formatter = self.output.get_formatter(output_type)
self.output.out(cmd_result, formatter=formatter, out_file=out_file)
self.raise_event(EVENT_CLI_SUCCESSFUL_EXECUTE)
Copy link
Member

Choose a reason for hiding this comment

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

two concerns:

  1. could the new event bring benefit to CLI if exit code = 0 could represent successful execution?
  2. should SUCCESSFUL_EXECUTE be raised after cmd_result is returned? It seems that in current code output format failure would block the event.

Copy link
Member Author

Choose a reason for hiding this comment

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

could the new event bring benefit to CLI if exit code = 0 could represent successful execution?

I couldn't really get your question. Could you elaborate?

should SUCCESSFUL_EXECUTE be raised after cmd_result is returned?

No. If self.output.out failed, this is considered a failure and SUCCESSFUL_EXECUTE shouldn't be raised by design.

@@ -219,6 +219,7 @@ def invoke(self, args, initial_invocation_data=None, out_file=None):
if cmd_result and cmd_result.result is not None:
formatter = self.output.get_formatter(output_type)
self.output.out(cmd_result, formatter=formatter, out_file=out_file)
self.raise_event(EVENT_CLI_SUCCESSFUL_EXECUTE, result=cmd_result.result)

Choose a reason for hiding this comment

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

Should we raise the event only when exit_code is 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. On the other hand, EVENT_CLI_POST_EXECUTE is raised unconditionally.

knack/knack/cli.py

Lines 233 to 234 in 61057a3

finally:
self.raise_event(EVENT_CLI_POST_EXECUTE)

Choose a reason for hiding this comment

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

Then should you check the exit_code from

exit_code = self.result.exit_code

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really necessary.

cmd_result = self.invocation.execute(args)

will raise an exception first if exit_code is not 0.

Also,

self.output.out(cmd_result, formatter=formatter, out_file=out_file)

already assumes the command is successfully executed and exit_code is 0.

Choose a reason for hiding this comment

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

got it.

@jiasli jiasli merged commit e0fec24 into microsoft:master Dec 16, 2020
@jiasli jiasli deleted the hint branch December 16, 2020 08:12
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