albrow / jobs

A persistent and flexible background jobs library for go.
MIT License
501 stars 47 forks source link

FindById() doesn't return error for missing jobs #18

Closed epelc closed 9 years ago

epelc commented 9 years ago

FindById() will return an empty job struct(with the id set to whatever you gave it) and nil if the id doesn't exist.

Example

j, err: = jobs.FindById("Hello world")
if err != nil {
    panic(err)
}
log.Println("Found job:", j, err)

@albrow It looks like transaction.exec() doesn't check for an empty set/list error.

epelc commented 9 years ago

I found and fixed this I'll submit a pr soon

albrow commented 9 years ago

@epelc thanks for reporting this. Looking forward to the PR.

I did this in zoom and must have forgotten or messed it up here.

epelc commented 9 years ago

@albrow I was going to add/replace the following here

} else if len(fields) == 0 {
        return errors.New("jobs: In scanJob: Job not found")
    } else if len(fields)%2 != 0 {
        return fmt.Errorf("jobs: In scanJob: Expected length of fields to be even but got: %d", len(fields))
    }

Do you want to fix it at a higher level instead? I don't really know 100% how your transaction handler works yet.

albrow commented 9 years ago

Thanks, that's exactly the right place to do it. The error you return from scanJob will bubble all the way up to FindById and any other function/method that calls scanJob.

However, instead of returning a generic error, we should return a specific error type. It's important for callers to be able to differentiate between a job not existing and a problem connecting to the database, because the two cases might be handled differently.

Look at ErrorNameAlreadyRegistered for how I did a specific error type. Probably call this one ErrorJobNotFound.

Alternatively if you just want to submit code that returns a generic error, I can implement the error type myself.

epelc commented 9 years ago

I forgot about that I'll switch it over. Do you mind if it's just a var? I don't think there's any extra data we need to return with it.

albrow commented 9 years ago

Thanks! I commented with feedback on https://github.com/albrow/jobs/pull/19 directly.

epelc commented 9 years ago

Will do