Skip to content

[generic-config-updater] Logging change just before applying it #1934

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.

Sign up for GitHub

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

Merged
ghooo merged 2 commits into from
Nov 18, 2021
Merged

[generic-config-updater] Logging change just before applying it #1934

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b1aae4
[generic-config-updater] Logging change just before applying it
ghooo Nov 17, 2021
93b8ed1
removing if-else
ghooo Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion generic_config_updater/generic_updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,12 @@ def apply(self, patch):
self.logger.log_notice(f" * {change}")

# Apply changes in order
self.logger.log_notice("Applying changes in order.")
if changes_len:
self.logger.log_notice(f"Applying {changes_len} change{'s' if changes_len != 1 else ''} in order:")
else:
self.logger.log_notice(f"No changes to apply.")
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Nov 17, 2021

Choose a reason for hiding this comment

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

To make it simple, how about

            self.logger.log_notice(f"Applying {changes_len} change(s) in order:")

And no need to use if-else.

Copy link
Copy Markdown
Contributor Author

@ghooo ghooo Nov 17, 2021

Choose a reason for hiding this comment

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

I prefer log messages to be very informative even if they need a little extra code.

In case of 0, I prefer printing a clear message saying

No changes to apply.

In case of 1 change:

Applying 1 change in order:

In case of many changes:

Applying x changes in order:

I think this makes the logs easier to read.

Copy link
Copy Markdown
Contributor Author

@ghooo ghooo Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can make it

self.logger.log_notice(f"Applying {changes_len} change{'s' if changes_len != 1 else ''} in order{':' if changes_len > 0 else '.'}")

less informative but not by much, but still easy to understand, and consistent with the previous list.

Copy link
Copy Markdown
Contributor Author

@ghooo ghooo Nov 17, 2021

Choose a reason for hiding this comment

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

updated

for change in changes:
self.logger.log_notice(f" * {change}")
self.changeapplier.apply(change)

# Validate config updated successfully
Expand Down