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

Refactor execute to use wrapper for logging #2975

Merged
merged 1 commit into from Nov 18, 2020
Merged

Refactor execute to use wrapper for logging #2975

merged 1 commit into from Nov 18, 2020

Conversation

ssbarnea
Copy link
Member

Avoid each implementation of Command to call logging and use a wrapped to log executions of each subclass.

@cognifloyd
Copy link
Member

cognifloyd commented Nov 17, 2020

These commands currently do not call self.print_info():

  • init/scenario.py
  • init/role.py
  • list.py
  • login.py
  • matrix.py
  • test.py

So they probably need a dummy print_info() impl:

    def print_info(self):
        pass

@ssbarnea
Copy link
Member Author

@cognifloyd They do not need any calls because any class inheriting from command.Base will call the print_info() from the base class. That is what this change is adding as we no longer need to call it in each implementation. execute() is wrapped dynamically with the logging.

"Running ..." was present with list, login,

There are some unrelated problems with "matrix" and "check", "test" and "converge" do not produce their own sections because they are sequences, but each command within them is displayed and that was the goal.

I am not saying that the "sequences" should not print sometime but because section support in terminal is likely not nested, we would break the rendering if we would display nested ones.

We clearly need to find a way to display the sequences in a different section when we do --help, so we can decouple them from the basic commands.

Avoid each implementation of Command to call logging and use
a wrapped to log executions of each subclass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants