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

Fixing colorama global init in print_result #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

geg347
Copy link

@geg347 geg347 commented Jun 29, 2022

Fixing issue #29

Hello,
Here we proposed a fix for the issue 29.
Thanks and have a great day!

Best regards,
geg347

@dbarrosop
Copy link
Contributor

Thanks for the PR, two things:

  1. shouldn't print_result include the same change?
  2. Looks like tests will need to be fixed as the output is probably differing a bit (probably the escape codes changed slightly)

@geg347
Copy link
Author

geg347 commented Jun 30, 2022

Hello,
You meant print_title.
I just added the fix to this function.
It may fix the pipeline as the init function wasn't called in the print_title function. Now it is.

I have a question : should I also add the deinit function in this file (https://github.com/nornir-automation/nornir_utils/blob/master/nornir_utils/plugins/processors/print_result.py).
I don't know where to put this though. init looks a bad idea...
Thanks

@dbarrosop
Copy link
Contributor

I am starting to think we should try a different approach to make this more robust. I think the main issue is due to the kwargs being passed to init:

init(autoreset=True, strip=False)

I suspect that if we remove autoreset=True the problem will go away. Would you mind testing? That would mean though we need to add Style.RESET_ALL at the end of every print but I believe we should do that and avoid messing with stuff that might have a side-effect outside of the function itself. Thoughts?

@geg347
Copy link
Author

geg347 commented Jul 4, 2022

Hello,
I just tested with
init(strip=False)
There are no more unwanted characters.
But the display of a Nornir task is broken (it is all the same color).
I also tried to add
print(Style.RESET_ALL)
After the print calls in print_title and print_result functions and it is better but the output still differs from original code.
What do we do? We keep the first changes or you want to investigate further, please?
Thanks!

@dbarrosop
Copy link
Contributor

dbarrosop commented Jul 4, 2022

I think we should investigate further and fix the issue completely instead of trying to workaround it. What I think we need to do is add the reset to all prints, for instance:

from:

   print("{}{}{}{}".format(Style.BRIGHT, Fore.GREEN, msg, "*" * (80 - len(msg))))

to:

   print("{}{}{}{}{}".format(Style.BRIGHT, Fore.GREEN, msg, "*" * (80 - len(msg)), Style.RESET_ALL))

the only pain is we will have to add to all print statements

@geg347
Copy link
Author

geg347 commented Jul 4, 2022

Same behaviour with Style.RESET_ALL at the end of all print statements.
The display of a Nornir task is still broken (it is all the same color).

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