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 shellcheck CI check + address its concerns in few bash scripts found #272

Merged
merged 9 commits into from
Mar 3, 2024

Conversation

yarikoptic
Copy link
Contributor

I (newbie to slackdump) was looking for incremental backup setup. Ran into bash script -- decided to contribute but also to robustify before I "mass try it" since shellcheck is right in warning (generally ;-) ) about such constructs as ( $(im_channels) ) since they might not handle correctly cases where names have spaces. I do not have proof yet though here that it would apply. Neither I have yet tested in this script that the helper append_to_array works.

but here is a script which demonstrates problem and checks different solutions for it
#!/bin/bash
# Disabling shellcheck check here since adding escaped symbols for demo
# shellcheck disable=SC2028

function func {
    echo word
    echo "phrase with space"
    echo 'wordwith\n'
}

function func2 {
    echo 'last-word\r'
}

function append_to_array {
    local array_name="$1"
    shift
    #local cmd="$@"

    # Read output of the command into a temporary array, splitting on newline
    # shellcheck disable=SC2034
    if [ -n "$*" ]; then
        IFS=$'\n' read -r -d '' -a temp_array < <("$@")
    else
        # no command provided -- read from stdin
        IFS=$'\n' read -r -d '' -a temp_array
    fi

    # Use eval to safely append temp_array to the named array
    eval "$array_name+=(\"\${temp_array[@]}\")"
}

function show {
    for i in "${LIST[@]}"; do
        echo "<$i>"
    done
}

echo "Can't use mapfile since not portable -- not available on OSX"

echo "Simple array: not good -- would not be good for spaces"
LIST=( $(func) )
show

echo "Tuned read: not good for appends to array"
IFS=$'\n' read -r -d '' -a LIST < <(func)
IFS=$'\n' read -r -d '' -a LIST < <(func2)
show

echo "Custom helper: good -- we will append to last listed but tested to work if there no array yet"
append_to_array LIST func
show

echo "Trying more complicated case with a pipe"
LIST=()
append_to_array LIST < <(func | tr o X)
show

…ce long line

shellcheck was complaining

	In contrib/messages_json_parsing/jq/find_matches_print_dates_and_text.sh line 5:
	cat "${SAMPLE}" | jq '.messages[] | select(( .text != null) and (.text | test("JoInEd";"i"))) | (.ts |= (tonumber|todate)) | .ts, .text' | tr -d '"'
		^---------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

	In contrib/messages_json_parsing/jq/print_messages.sh line 5:
	cat "${SAMPLE}" | jq '.messages[]|.user +": "+.text' | tr -d '"'
		^---------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "shellcheck -f diff contrib/incremental_backup/dump.sh | patch -p1",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
… and assign at the same time

Also made that script executable since it is a script with shebang -- should be
good like that

Here is exemplar list of complaints from shellcheck.  Not sure if "local" was actually needed here.
The mapfile ones are still to be done

    In contrib/incremental_backup/dump.sh line 82:
        LIST+=($(echo "$CHANNEL_LIST_JSON" | $JQ_B -r 'map(select(.is_im == true)) | .[] | .user, .id'))
                   ^-- SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).

    In contrib/incremental_backup/dump.sh line 127:
        local META_JSON=$(<"$META_FILE")
                  ^-------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

    In contrib/incremental_backup/dump.sh line 129:
        local PREVIOUS_DATE="$(echo "$META_JSON" | $JQ_B -r '.last_updated | select(. != null)')"
                  ^-----------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

    In contrib/incremental_backup/dump.sh line 156:
        local NEW_MESSAGE_COUNT=$($JQ_B -r '.messages | length' "$CHANNEL_FILE")
                  ^---------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

    In contrib/incremental_backup/dump.sh line 165:
                local MERGED_CONTENT=$($JQ_B -s '.[0] as $o1 | .[1] as $o2 | ($o1 + $o2) | .messages = ($o1.messages + $o2.messages)' "$CHANNEL_FILE_OLD" "$CHANNEL_FILE")
                                  ^------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.
                                                            ^-- SC2016 (info): Expressions don't expand in single quotes, use double quotes for that.

    In contrib/incremental_backup/dump.sh line 174:
        local TOTAL_MESSAGE_COUNT=$($JQ_B -r '.messages | length' "$CHANNEL_FILE")
                  ^-----------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

    In contrib/incremental_backup/dump.sh line 190:
        LIST+=( $(im_channels) )
                    ^------------^ SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).

    In contrib/incremental_backup/dump.sh line 192:
        LIST+=( $(group_channels) )
                    ^---------------^ SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).
@rusq
Copy link
Owner

rusq commented Mar 3, 2024

Hey @yarikoptic, thanks for your contribution!

Copy link
Owner

@rusq rusq left a comment

Choose a reason for hiding this comment

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

LGTM

@rusq
Copy link
Owner

rusq commented Mar 3, 2024

I have left some comments in the other PR that you have submitted, if you could have a look at it too: #271

@rusq rusq merged commit 61a73d0 into rusq:master Mar 3, 2024
2 checks passed
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