Better error messages

[Twister915]

trafficstars

Apologies if this is not the correct process for submitting a pull request.

I was having the following issue when trying to learn the drone CLI:

I'd like to improve the error messages, so I've changed them to this for all build commands:

I've added a util.go file to the build package, which contains the following two functions:

// parses a string into a build #, with no support for resolving the latest build
// returns very friendly errors as opposed to strconv errors
func parseBuildArg(arg string) (buildNumber int, err error)

// parses a string, which is one of "last" or an exact build number.
// the returned value is the latest build, it's number 
//
// this is redundant but more often than not you don't actually want/need the actual build, 
// just it's number; you can use _ to throw away the build and 'number' to access just the number
//
// this is an example
//
// _, number, err := getBuildWithArg(c.Args().Get(1), owner, repo, client)
//
func getBuildWithArg(arg, owner, repo string, client drone.Client) (build *drone.Build, number int, err error)

Lastly, in the special case of stopping a build, the job number is also accepted. I made so if there is no specified job, then the job defaults to ID 1 (which is similar to how it works now), but if there's a malformed integer specified, then the error is printed.

[bradrydzewski]

Instead of a helper function for parsing the build number perhaps instead providing a more descriptive, shared error message?

+ var errInvalidBuildNumber = errors.New("Missing or invalid build number")

***************

	number, err := strconv.Atoi(c.Args().Get(1))
  	if err != nil {
-  		return err
+  		return errInvalidBuildNumber
  	}

[Twister915]

@bradrydzewski The problem with that is we'd have to do a few conditional checks in every place the build number is parsed to get different error messages for not specifying a build number and specifying a malformed build number. The code, in all places, would look a little something like this:

      buildNumberString := c.Args().Get(1)
      if len(buildNumberString) == 0 {
            return ErrNoBuildNumberSpecified
      }
      number, err := strconv.Atoi(buildNumberString)
      if err != nil {
            return ErrInvalidBuildNumber
      }

Also, it's helpful to specify the exact error from atoi when we're printing it, so ErrInvalidBuildNumber would need to become a function which returns the "wrapped error" from atoi.

The helper function I've put in also accomplishes the shared and descriptive goal that you're interested in, since it's defining the error in the helper function. We could, of course, move that out to a package variable if you'd like to. I see an instance (parsing the job ID) where that'd certainly be helpful.

The way I'm seeing it- to maintain the same level of functionality, we'd have to add a good chunk more code and still have at least one helper function for wrapping that error consistently. That makes something like my helper function the most succinct way to approach this, but I might just not be seeing it correctly.

Thoughts?

[bradrydzewski]

I think Missing or invalid build number is descriptive enough. I'm not saying there is anything wrong with your code. It is just my personal preference to opt for a more generic error message and simpler code (less indirection).

Also we are considering moving to https://github.com/alecthomas/kingpin which has strongly typed arguments and enforces required values, in which case the tool will end up taking care of this for us.

I made so if there is no specified job, then the job defaults to ID 1

This makes sense

[Twister915]

@bradrydzewski Alright, that's fair enough. I'll change things over to that real quick.

[Twister915]

I've fixed it to be like you specified. Just read your comment about kingpin. Since it's already written, might as well have the errors for the time being.

(had a question here, but I figured it out myself a few minutes later- my bad)

[Twister915]

In build_stop.go since you had it in Atoi I thought you meant to do that and check if it's 0. I realize now that probably won't work, and I probably misinterpreted what that meant, so I'm going to change that despite it being a bit more complex. Anything wrong with this?

var job int
jobArg := c.Args().Get(2)
if jobArg == "" {
    job = 1
} else {
    job, err = strconv.Atoi(jobArg)
    if err != nil {
        return errInvalidJobNumber
    }
}

[tboerger]

And I think if you rebase the PR to resolve the conflicts we can finally get it merged.

[Twister915]

I think I was able to reconcile the changes between May and today, but I'm not 100% sure it's perfect.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bot-harness]

This PR has been automatically closed due to inactivity.