Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support colored output on screen && add new level: SUCCESS #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Miatec
Copy link

@Miatec Miatec commented Jan 4, 2020

I've unnexpectedly found the time to develop the feature I proposed you (issue #1 ).

3 files where modified:

  • src/logger.sh - obviously to add the feature
  • test/logger.sh - to test the feature and add a test on the syntax validity of src/logger.sh
  • readme.md - to add the description of the new feature.

Copy link
Owner

@adoyle-h adoyle-h left a comment

Choose a reason for hiding this comment

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

Thanks for your commits. I have reviewed it and left some comments.

if [[ "${ARRAY_LEVEL_COLOR[$level]+isset}" ]]; then
return 0
else
return 1
Copy link
Owner

Choose a reason for hiding this comment

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

You should use "echo yes/no" instead of "return 0/1" to represent the exit status of function. Because the process will be terminated with non-zero exit code when current shell has set set -o errexit.

_levelExists $level
local levelExists=$?

if [[ "$levelExists" == "0" ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

_levelExists function is unnecessary.
I think if [[ "${ARRAY_LEVEL_COLOR[$level]:+isset}" ]]; then is simple enough.

if [[ -n "$LOG_TARGET" ]] ;then
echo "$msg" | tee >> "$LOG_TARGET"
else
echo "$msg"
local levelColor=$(_getLevelColor $level)
echo -e "${levelColor}${msg}${BASH_COLOR_CODE_DEFAULT}"
Copy link
Owner

Choose a reason for hiding this comment

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

msg may contain ANSI escape sequences.
Use printf '%b%s%b\n' "${levelColor}" "${msg}" "${BASH_COLOR_CODE_DEFAULT}".


function _getLevelColor(){
Copy link
Owner

Choose a reason for hiding this comment

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

Put this function definition before _echo.

Copy link
Owner

Choose a reason for hiding this comment

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

The function definition should be function name() {, not function name(){.

@@ -52,7 +74,7 @@ function _log() {
level="${2-${FUNCNAME[1]}}"
date_time=$(_date_time)
function_name="${FUNCNAME[2]}"
_echo "[$date_time][$level]($function_name) $msg"
_echo $level "[$date_time][$level]($function_name) $msg"
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the trailing space.

local msg=$1
local msg=$2
local level=$1

if [[ -n "$LOG_TARGET" ]] ;then
echo "$msg" | tee >> "$LOG_TARGET"
Copy link
Owner

Choose a reason for hiding this comment

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

I think it should support colored output to screen, while no-colored output to file.

printf '%b%s%b\n' "${levelColor}" "${msg}" "${BASH_COLOR_CODE_DEFAULT}"
echo "$msg" >> "$LOG_TARGET"

{
# bash -n check the syntax of logger.sh.
# If there is an error, it will stop the test script right now.
bash -n "$CUR_FILE_DIR"/../src/logger.sh ""
Copy link
Owner

Choose a reason for hiding this comment

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

Because of set -o errexit at line 3, the process will be terminated immediately when this line return non-zero exit code, and it will not reach line 18.



function loggerShCheck()
{
Copy link
Owner

Choose a reason for hiding this comment

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

Use function loggerShCheck() {. Follow the coding style.


Print message to `stdout` and write it to a file: `source logger.sh "<output-path>"`.
*Note: No color of line in this mode*
Copy link
Owner

Choose a reason for hiding this comment

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

I think it should support colored output to screen, while no-colored output to file.




function colorCheck() {
Copy link
Owner

Choose a reason for hiding this comment

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

This function is unnecessary. The foo function will print colored texts.

@adoyle-h
Copy link
Owner

adoyle-h commented Jan 5, 2020

image
Please sign the DCO.

@adoyle-h adoyle-h changed the title Implementing issue #1 "Colors on terminal". Support colored output on screen && add new level: SUCCESS Jan 5, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants