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 write results function #18

Conversation

timeforplanb123
Copy link

write_results:

  • function writes Result objects to files with hostname names
  • write_result returns list of tuples with hostname + diff between old file and new file
  • parameters:
    • dirname - dirname you want to write the results. write_results creates dirs with filenames (hostnames) if it's necessary
    • vars, failed, severety_level
    • write_host - write hostname to file or not
    • count is the same that count from print_result. It's number of sorted results (-5 or 5, for example). 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.
    • append - "a+" or "w+" mode
    • no_errors - dont't write result with errors to files

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_stat prints statistic for Result object
write_result writes Result object to file
write_results writes Result objects to files with hostname names
@dbarrosop
Copy link
Contributor

Hi, nice work. A couple of comments before diving into actual reviews:

  1. Looks like your PRs got a bit messy as each PR you opened includes the commits from the previous one. This will become a problem as we start reviewing them and asking for minor changes as then you will run into conflicts.

  2. It'd be great if you could write jupyter notebooks testing and demonstrating them so they can be included in the docs. For instance:

    1. https://github.com/nornir-automation/nornir_utils/blob/master/docs/source/tutorials/function_print_result.ipynb
    2. which leads to https://nornir.tech/nornir_utils/html/tutorials/function_print_result.html

    These notebooks both demonstrate what they do and also serves as tests.

add new count parameter to print the number of results. Useful for a lot of identical results
print_stat prints statistic for Result object
write_result writes Result object to file
add new count parameter to print the number of results. Useful for a lot of identical results
print_stat prints statistic for Result object
write_result writes Result object to file
write_results writes Result objects to files with hostname names
@dbarrosop
Copy link
Contributor

I think your PRs are still a bit messy with commits from different PRs landing on all of them

@timeforplanb123
Copy link
Author

timeforplanb123 commented Oct 21, 2021

4 PR, one after another, because I wanted to merge without conflicts readme.md, __init.py__ + each function has an import of _slice_result function from print_result.

It seemed to me that it would be convenient :)
I update every PR to exclude errors.
We can work/test/merge PR in the order of PR creation and everything should be fine.

Although, there are really a lot of redundant commits. It looks dirty. Then, maybe, can i join all of the new features into one commit/pr, based on master?

@dbarrosop
Copy link
Contributor

It is not about being cool. A PR should only contain what it is meant to contain. By introducing commits from other PRs the review process becomes more difficult and cumbersome as we need to figure out what’s what. This PR now has 12 commits and it will surely have more by the time we are done with all of the PRs.

It also blocks the PR as it can’t be merged if the others aren’t merged. Fixing conflicts like the ones in the README are also very easy to fix and can even be done in the GH UI, however, having to sync commits and changes between all the PRs is probably going to end up being a lot of work for you.

If you prefer to do it like this I am good with it but then we are going to be reviewing PRs one by one and ignore the rest until the "parent" is merged. Just let me know how you want to proceed.

@timeforplanb123
Copy link
Author

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