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

Add new count parameter: #15

Conversation

timeforplanb123
Copy link

update print_result function:

  • new parameters:
    • add new count parameter. It's number of sorted results. It can be useful for a large number of identical results. It's acceptable to use numbers with minus sign(-5 as example), then results will be from the end of results list
    • print_host - print host or not

add new count parameter. It's number of sorted results. It can be useful for a large number of identical results. It's acceptable to use numbers with minus sign(-5 as example), then results will be from the end of results list.
@dbarrosop
Copy link
Contributor

dbarrosop commented Oct 19, 2021

Hi, thanks for the PR, in order to better review it I have a few initial comments/questions:

  1. You need to format the code with black
  2. It is hard to understand the changes based on the documentation and the code changes. Would you mind clarifying what specifying count is supposed to do?
  3. What is the reasoning for exposing print_host?

add new count parameter to print the number of results. Useful for a lot of identical results
add new count parameter to print the number of results. Useful for a lot of identical results
@timeforplanb123
Copy link
Author

  1. omg! I'm sorry for these stupid mistakes. Fixed! :'(
  2. When you have a lot of number of identical results and you want to output them to the terminal, it is convenient and sufficient to output only part of them. The count parameter outputs the specified number of sorted results from the beginning of the results list or from the end. Sometimes, it's useful + I use it in nornir_cli and would like to just import it.

As instance:

print_result(result)
netmiko_send_command************************************************************
* dev_1 ** changed : False *****************************************************
vvvv netmiko_send_command ** changed : False vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv INFO
2021-10-20 14:47:22
Wednesday
Time Zone(Moscow) : UTC+03:00
^^^^ END netmiko_send_command ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* dev_2 ** changed : False *****************************************************
vvvv netmiko_send_command ** changed : False vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv INFO
2021-10-20 14:47:22
Wednesday
Time Zone(Moscow) : UTC+03:00
^^^^ END netmiko_send_command ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
print_result(result, count=1)
netmiko_send_command************************************************************
* dev_1 ** changed : False *****************************************************
vvvv netmiko_send_command ** changed : False vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv INFO
2021-10-20 14:47:22
Wednesday
Time Zone(Moscow) : UTC+03:00
^^^^ END netmiko_send_command ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
print_result(result, count=-1)
netmiko_send_command************************************************************
* dev_2 ** changed : False *****************************************************
vvvv netmiko_send_command ** changed : False vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv INFO
2021-10-20 14:47:22
Wednesday
Time Zone(Moscow) : UTC+03:00
^^^^ END netmiko_send_command ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  1. Agree. There is no reason :) Fixed!

if isinstance(count, int):
length = len(results)
if count >= 0:
_ = [0, length and count]
Copy link
Contributor

@dbarrosop dbarrosop Oct 21, 2021

Choose a reason for hiding this comment

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

_ as a var has different meanings depending on context (most common one being "please, ignore") so I'd suggest not using it

@dbarrosop
Copy link
Contributor

dbarrosop commented Oct 22, 2021

So if I understood you correctly, the basic idea is to "hide" identical results to simplify the output. I think this is very useful and I am completely on board with this feature. However, looking at the PR I am not sure these changes achieve such goal.

Imagine the following situation. I run some task over 100 devices that I expect to lead to the same result so I run print_result(result, count=1):

  1. How is that different from print_result(result["rtr00"])?
  2. If you know they are all different, why print more than one?
  3. What happens if some random routers fail? Inspecting the code it looks like those will be hidden from the user.
  4. What happens if I am wrong and some results are actually different from the one I am printing?

I think if we want to really implement this sort of functionality we need to:

  1. Make sure errors aren't "hidden"
  2. Compare results with each other and print all of the results that are actually different, indicating which hosts had which output. So if you have 4 different results for 100 devices you print all four results indicating which hosts had which output.

Rather than implementing my suggestion as part of print_result though it might be cleaner to have some form of "summarizing" function that performs the filtering/aggregation and just call print_result directly (and it might be more reusable too as it might be leveraged for other purposes too)

Did I misunderstand anything? Thoughts?

@timeforplanb123
Copy link
Author

It's a good idea, but my idea is much simpler. I just wanted to be able to limit the amount of output to the terminal without changing the terminal settings (scrollback settings), but at the same time see some results.

If there are errors on some hosts, then to avoid looking at the entire print_result output, for example, for 100 hosts, there is a new print_stat function that will show the task execution status for each host. You can combine print_result and print_stat for optimal output of results.
For example, output the last 5 results to see some results and view statistics for all 100 hosts.

I replaced all new functions to single PR with ipynb examples #21

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