search

xanzy / go-gitlab

GitLab Go SDK
Apache License 2.0
2.39k stars 947 forks source link

Fix `GetSinglePersonalAccessTokenByID` arg name #1954

Closed theipster closed 3 months ago

theipster commented 3 months ago

Calling the token ID user is extremely confusing...!

Relates to #1686.

theipster commented 3 months ago

@svanharmelen Thanks for the quick response! 🙂

In my opinion you shoud always check errors, also in tests. So please revert those changes.

Updating the error message to make it more descriptive and applicable to the actual error is of course fine.

I'm happy to update my PR, but can you please explain what the purpose is / what error you might expect to see in these specific lines of code, given that it's essentially calling a standard library function with fixed arguments (so the return value will always be fixed)? Error checking would make sense if the arguments were variable, but that's not the case here.

Also, the TestRotatePersonalAccessToken test doesn't have error checking either.

svanharmelen commented 3 months ago

I'm not going to spent too much words on it, but I believe that you should always check all your errors everywhere. As soon as you start to reason about which error checks make sense and which do not, you'll be on a slippery slope just waiting for a missed error check to bites you in production when you least expect it.

Next to that it will set a precedent for other users or committers of this package or maybe even to people who are new at Go. I prefer to encourage them to get used to just always check all errors as in my opinion the Go language best practices also encourages you to do so.

If TestRotatePersonalAccessToken doesn't do error checking then that one slipped through unintentionally. So feel free to update that test instead in order to fix it and make everything consistent again.

theipster commented 3 months ago

@svanharmelen I've now removed the changes regarding the tests.

As a general best practice, I understand and agree with your intentions. However, in this particular case, I'm struggling to articulate what the error message should actually be (and PersonalAccessTokens.ListPersonalAccessTokens returned error:" is definitely not accurate because it has nothing to do with the ListPersonalAccessTokens function), so I'll leave it for somebody smarter than me to rename the error message.