refactor: make NewClient return an error

[sruehl]

This PR addresses the TODO Note: Starting with v0.5 this function will return an error. and let NewClient return an error.

Note on the PR: I use github.com/stretchr/testify for error assertion with seems IMHO pretty standard in the go projects I worked with. If this is unwanted I could rework this.

resolves #621

[sruehl]

@magiconair I worked on a TODO you defined in source. Not sure if there is a ticket for that. If I find it I link it here. (Edit: found it and linked)

[magiconair]

Thank you! I've missed that part with the move to v0.5. Please do not check the error for the c.Close() method everywhere since we cannot do anything with that error anyway. There is no way to recover. We just have this in the signature to be consistent.

As for testify. I went down that route before and decided against this since it would introduce the third testing framework into the code base. Yes, it is more convenient but it doesn't justify the cost of having different code styles in different parts of the code base. So please refactor this to look like the rest (i.e. got,want style and maybe use verify.Values)

[sruehl]

Oh and before I forget, take a closer look (@magiconair) what I did with errors. So I collect the errors there and return them with join. Could also be done via early exit. Also I wrap errors with %w. Maybe this is unwanted too.

[magiconair]

I'll have a look. As a guiding principle I prefer consistency when we make changes like this. I'm OK with changing direction but then let's do it all over the place. And in a separate PR.

[sruehl]

I'll have a look. As a guiding principle I prefer consistency when we make changes like this. I'm OK with changing direction but then let's do it all over the place. And in a separate PR.

Clarifying question on that: I assumed that comment was related to wrapping with %w. Should I switch from errrors join to early exit too?

[magiconair]

Final nitpick about a variable name but then this LGTM.

[magiconair]

As an aside: I've done the force-push update as well earlier in my career and while that keeps the commit cleaner it makes reviewing harder since I don't see what you have changed and always have to review the full change set. I've come to settle on individual commits and leave it to the maintainer to squash the commits if they want.

[sruehl]

Usually I don't force push either but I read somewhere that it was requested to squash commits before merge so I started doing that from the start. But yes I agree: that makes following changes hard

[magiconair]

On this project? I'll have a look or can you point me to it...

[sruehl]

Maybe my mind tricked me... I'll stick to normal commits for future PRs (or look if I can find that comment) 👍🏼
Btw just in case you didn't notice the little compare button on the right shows the diffs on force pushes too which is quite helpful

[magiconair]

You'll learn something new every day :)