-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
[enhancement] Move logger call into subroutine #57
Comments
@feckert Here subroutine refers to creating a specific function for logger calls? |
@devkapilbansal In the described scripts in my description there is no function for the logger call. |
@devkapilbansal I have noticed that there are quite a few instances of the calls to I think this would make the code more readable since we have many calls to the logger. |
Yes, I remove |
@devkapilbansal one more thing, regarding verbose mode, I think we could handle the verbose output using this function we're imagining here. We have a lot of if/else in the code for verbose messages which are harming the readability of the agent code, what about solving this with our function? Eg:
Practical examples below. Message shown regardless of mode
Result is shown both in verbose mode and non verbose:
Message + verbose appended
Result seen in non-verbose mode:
Result seen in verbose mode:
Verbose message only
Result seen only in verbose mode (in non-verbose mode the log line is not executed at all):
Non verbose message
Non verbose result:
Verbose result:
|
What do you think about adding a new "debug mode" to turn this on?
In verbose mode we could avoid using it. In debug mode we would know (and document) that any output is sent to the logging facility, causing some output to be repeated, Or alternatively, we could even modify our log function to use echo instead of |
Can we do something like this 👇
When |
Sounds good, but how can all the cases mentioned in #57 (comment) be covered by this? Can you please provide some examples of how it's going to look like? |
Here are some examples on how it will work: Message shown regardless of mode
The result will be shown in both verbose and non-verbose mode
Verbose mode only
Result:
I don't take the other two cases into consideration as we are handling them in if-else to stop redundant messages in non-verbose mode as found here. openwrt-openwisp-monitoring/openwrt-openwisp-monitoring/files/monitoring.agent Lines 146 to 157 in 5460f01
I added a commit to show how I am approaching this: d965c42 |
Ok but what about all the other cases?
I think -n should precede the message, eg:
Same here, I think -v should precede the message, eg: So if we need to display a message that in verbose mode should contain more info we may be able to do:
These combinations should cover all the most critical cases, if there are other cases to handle we could just simplify. What do you think? |
@jedboywonder these messages are out of topic here, I am going to delete them. Django can be configure to send logs to a log collector. Use the general chat to ask questions. |
The
logger
shell call is used over and over again.Which makes the code confusing.
I would suggest that you move the call to a subroutine.
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.init
https://github.com/openwisp/openwrt-openwisp-monitoring/blob/master/openwrt-openwisp-monitoring/files/monitoring.agent
The text was updated successfully, but these errors were encountered: