mscs icon indicating copy to clipboard operation
mscs copied to clipboard

[WIP] Fixshellcheck

Open sandain opened this issue 3 years ago • 8 comments

This is a work in progress to fix all of the shellcheck errors/warnings/etc.

sandain avatar May 03 '21 22:05 sandain

I've cleaned up some of the issues found by shellcheck. There are a bunch more!

My tests (up to commit cfe1c565158138cf59d02286e64450fdaf7328e6) show that everything seems to be working still, but this is going to need more testing..

sandain avatar May 04 '21 01:05 sandain

I merged the changes that I had been working on but had not yet committed. Things seem to be working, but we need to do some serious testing with all of this...

We have a few more issues to tackle, but with these commits, we are getting close.

sandain avatar May 25 '21 21:05 sandain

A lot of the SC2181 errors can be fixed by changing the way we do error output, I think. For instance, we do something like below to propagate error messages to the user:

getCurrentMinecraftVersion() {
  [...]
  # Print an error and exit if the version string is empty.
  if [ -z "$VERSION" ]; then
    printf "Error detecting the current Minecraft version.\n"
    exit 1
  fi
  printf "%s" "$VERSION"
}
getClientVersion() {
  CURRENT_VERSION=$(getCurrentMinecraftVersion "$1")
  if [ $? -ne 0 ]; then
    printf "%s\n" "$CURRENT_VERSION"
    exit 1
  fi
  [...]
}

If we instead pipe these errors to STDERR, I think it would simplify the code:

getCurrentMinecraftVersion() {
  [...]
  # Print an error and exit if the version string is empty.
  if [ -z "$VERSION" ]; then
    printf "Error detecting the current Minecraft version.\n" 1>&2 
    exit 1
  fi
  printf "%s" "$VERSION"
}
getClientVersion() {
  CURRENT_VERSION=$(getCurrentMinecraftVersion "$1")
  [ -n "$CURRENT_VERSION" ] || exit 1
  [...]
}

This is untested...

sandain avatar May 25 '21 21:05 sandain

I got rid of some of the SC2181 errors by doing what I mentioned above. It seemed to work.

sandain avatar May 26 '21 18:05 sandain

Removing LOCAL broke a lot of things. I think I have a workaround...

sandain avatar Jun 25 '21 22:06 sandain

So removing LOCAL made all variables global. This caused subroutines to overwrite global variables. A POSIX compatible way to fix this was to simply start a new shell for each subroutine by swapping curly brackets with parens. See commit 5c81c6d9b4998c3b0b93083d66d9d855a3876089 for details.

With this change, I think we are closer to having a working script again. Please test this branch to see if we are ready to merge it into the main branch.

sandain avatar Jun 25 '21 23:06 sandain

We might need a rebase on the current main branch at this point in time.

zanix avatar Jan 01 '22 23:01 zanix

I'm tempted to NACK this pull request and start from scratch. I just set the shellcheck severity to "error" and fixed the few issues identified, so the main branch is now reporting no errors.

sandain avatar Jan 03 '22 19:01 sandain