evolix/evocheck
evolix/evocheck
21
2
Fork 2
Code Issues 48 Pull requests 5 Releases 37 Wiki Activity

Improve logging with optional verbose comment #71

Merged
benpro merged 4 commits from verbose-logging into master 2019-03-21 14:27:38 +01:00
Conversation 4 Commits 4 Files changed 1 +156 -156
jlecour commented 2019-03-20 22:41:23 +01:00
Owner
Copy link

I think the verbose mode is a really great idea and I'd like to go further.

I've never liked the low-level echo "IS_XYZ FAILED!" statement, duplicated everywhere, with optional verbose "message" juste after them.

I think we can have a single function to log failures and their optional comment.
If we need to change the way we log, there only one change to do.

In this first commit I've added the function and changed the failure logging only where there is a verbose call, to illustrate my idea. Obviously if this is confirmed as a good idea, every echo "IS_XYZ FAILED!" can be changed to failed "IS_XYZ" with a search/replace.

I think the verbose mode is a really great idea and I'd like to go further. I've never liked the low-level `echo "IS_XYZ FAILED!"` statement, duplicated everywhere, with optional `verbose "message"` juste after them. I think we can have a single function to log failures and their optional comment. If we need to change the way we log, there only one change to do. In this first commit I've added the function and changed the failure logging only where there is a `verbose` call, to illustrate my idea. Obviously if this is confirmed as a good idea, every `echo "IS_XYZ FAILED!"` can be changed to `failed "IS_XYZ"` with a search/replace.
benpro was assigned by jlecour 2019-03-20 22:41:33 +01:00
jlecour commented 2019-03-20 22:44:35 +01:00
Author
Owner
Copy link

Having a centralized logging function will also help to decide if we want to log to stdout or stderr, to a file…

Having a centralized logging function will also help to decide if we want to log to stdout or stderr, to a file…
benpro commented 2019-03-21 10:50:46 +01:00
Contributor
Copy link

LGTM

LGTM
benpro reviewed 2019-03-21 10:53:17 +01:00
evocheck.sh Outdated
@ -131,1 +128,3 @@
[ "${VERBOSE}" -eq 1 ] && [ -n "${msg}" ] && echo "${msg}"
# logging function
failed() {
test=$1
benpro commented 2019-03-21 10:53:08 +01:00
Contributor
Copy link

I don't like the name test for a variable. It could be the internal keyword test.
IMO we should choose another name.

I don't like the name `test` for a variable. It could be the internal keyword `test`. IMO we should choose another name.
evocheck.sh
@ -269,3 +276,2 @@
if is_installed apt-listchanges; then
echo 'IS_LISTCHANGESCONF FAILED!'
verbose "apt-listchanges must not be installed on Stretch"
failed "IS_LISTCHANGESCONF" "apt-listchanges must not be installed on Stretch"
benpro commented 2019-03-21 10:51:10 +01:00
Contributor
Copy link

Indeed, looks way better/cleaner.

Indeed, looks way better/cleaner.
benpro added the
enhancement
 label 2019-03-21 10:54:26 +01:00
jlecour commented 2019-03-21 12:48:28 +01:00
Author
Owner
Copy link

@benpro I've changed the variable names. You were right, it's better

@benpro I've changed the variable names. You were right, it's better
jlecour commented 2019-03-21 12:53:49 +01:00
Author
Owner
Copy link

And I've changed all call to echo with failed.

And I've changed all call to `echo` with `failed`.
benpro reviewed 2019-03-21 14:06:32 +01:00
evocheck.sh Outdated
@ -132,0 +129,4 @@
failed() {
check_name=$1
shift
checl_comments=$@
benpro commented 2019-03-21 14:05:55 +01:00
Contributor
Copy link

Typo I guess. :)

Typo I guess. :)
jlecour commented 2019-03-21 14:14:55 +01:00
Author
Owner
Copy link

fixed :/

fixed :/
jlecour changed title from WIP: Improve logging with optional verbose comment to Improve logging with optional verbose comment 2019-03-21 14:17:19 +01:00
Ghost approved these changes 2019-03-21 14:21:33 +01:00
benpro approved these changes 2019-03-21 14:27:07 +01:00
benpro closed this pull request 2019-03-21 14:27:38 +01:00
jlecour deleted branch verbose-logging 2019-03-21 20:38:16 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
bullseye

   
discussion

This need to be discussed

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
suggestion

Suggest to add/change something

  
wontfix

This won't be fixed

No labels
bug
bullseye
discussion
duplicate
enhancement
help wanted
invalid
question
suggestion
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
benpro
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/evocheck#71
No description provided.
Delete branch "verbose-logging"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?