MinecraftServerControl / mscs

Powerful command-line control for UNIX and Linux powered Minecraft servers
https://minecraftservercontrol.github.io
BSD 2-Clause "Simplified" License
479 stars 61 forks source link

[WIP] Fixshellcheck #290

Closed sandain closed 1 year ago

sandain commented 3 years ago

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

sandain commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

sandain commented 3 years ago

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

sandain commented 3 years ago

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.

zanix commented 2 years ago

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

sandain commented 2 years ago

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.