Improve nvm_download_artifact() process, fix #1291

[PeterDaveHello]

• Delete broken/checksum not matched local cache
• More output message.

[ljharb]

let's collapse this into the preceding line, to keep output minimal.

[PeterDaveHello]

hmmmm ... the reason why I put them into two is because ${TARBALL} is a little bit too long, what do you think?

[PeterDaveHello]

@ljharb ?

[ljharb]

I wouldn't really be concerned with line length - long lines wrap, and the important part would be in the front.

[ljharb]

also here; let's collapse this with the previous line. also, no space before the three dots :-)

[ljharb]

No need to set it to the string true; just true should be sufficient

[ljharb]

this line is redundant since line 1744 does the same thing

[PeterDaveHello]

@ljharb but line 1744 will be hidden, so the user will just see the progress bar without downloading info, is this fine?

[PeterDaveHello]

@ljharb ?

[ljharb]

i think rather than using USE_LOCAL_TARBALL, we could add nvm_echo "${TARBALL}" and return 0 to the previous "if" block, and then remove this nesting?

[PeterDaveHello]

Do you mean insert return 0 to this if block?
if nvm_compare_checksum "${TARBALL}" "${CHECKSUM}" >/dev/null 2>&1; then

Thanks

[ljharb]

i mean insert both of these:

nvm_echo "${TARBALL}"
return 0

[PeterDaveHello]

like this? I not sure about if I understand this part

    if nvm_compare_checksum "${TARBALL}" "${CHECKSUM}" >/dev/null 2>&1; then
      nvm_err "Checksums match! Using existing downloaded archive $(nvm_sanitize_path "${TARBALL}")"      USE_LOCAL_TARBALL=true
      nvm_echo "${TARBALL}"
      return 0
    else
      nvm_compare_checksum "${TARBALL}" "${CHECKSUM}"                                                     nvm_err "Checksum check failed!"
      nvm_err "Removing the broken local cache..."
      command rm -rf "${TARBALL}"
      nvm_err "Downloading ${TARBALL_URL} ..."
    fi

[ljharb]

yep! except that now you can change else to fi, remove the final fi, and de-indent the erstwhile "else" code

[PeterDaveHello]

Oops, I might not catch it, now I got your point 😄

[PeterDaveHello]

Updated!

[ljharb]

I wouldn't really be concerned with line length - long lines wrap, and the important part would be in the front.

[ljharb]

now that you've denested, we no longer need this variable at all - please remove it, and denest lines 1745-1761.

[PeterDaveHello]

oops, my fault ... thanks.

[PeterDaveHello]

Fixed.

[ljharb]

line 1739 and 1741 will both execute in this case - i think 1741 should use nvm_err and 1739 should be removed.

[PeterDaveHello]

Nice!

[PeterDaveHello]

Updated!