cycloidio / raws

[UNMAINTAINED] AWS Reader
MIT License
15 stars 0 forks source link

AWSReader methods return std error rather than specific error type #25

Closed ifraixedes closed 6 years ago

ifraixedes commented 6 years ago

In order to drive the refactoring by test failures and making the appropriated changes to fix the test failures, I needed to base this branch on the changes done in #24 hence, first review #24 before this one and once that's merged, I'll rebase this one from master.

About the changes of this branch, the subject of this PR with its associated issue #12 and the commit messages should be enough to know about it.

If you prefer to squash the commits of the refactoring of the methods (done, method by method), let me know and I'll do it just before merging for avoiding conflicts on rebases from the last master.

closes #12

coveralls commented 6 years ago

Coverage Status

Coverage increased (+4.1%) to 84.473% when pulling 780b9aaccb5f73aa5c091b2594cf93fdf00dd750 on if-12 into b7afb8027a69b9717c00909807d143424b9dac21 on master.

ifraixedes commented 6 years ago

I don't see any clear future that those 2 functions will have a more complicated logic.

About the *Error for me it's a smelly of bad design that an error is a pointer, that's why I mention to remove it, but ok.

I don't see it as if you want to use the type Error on whatever place and you want to use it as a pointer for being able to return nil, the function will work anyway.

xescugc commented 6 years ago

I don't see it as if you want to use the type Error on whatever place and you want to use it as a pointer for being able to return nil, the function will work anyway.

All the internal functions (IRC) use error interface as signature. So you can send nil already, the use case that you define, IMHO, seems really wrong code that's why it should not be handled.

ifraixedes commented 6 years ago

For example, in the case that a consumer of the library define its own error for some need that it has and embed the Error as in the following example

type MyErr {
    inner *Error
    code int
}

func (m MyErr) Error() string {
 // Whatever here
}

and it wants to use the function to do whatsoever, there is no need to check for if inner is nil before calling ErrorFom because you couldn't do ErrorFrom(*inner) without checking first is inner isn't nil otherwise it would be a panic, so I see that it's handy to be able to do ErrorFrom(inner) as pointer is accepted.

I don't see that's really wrong as you mentioned.

xescugc commented 6 years ago

Well we disagree with it :), because a use could pass to ErrorFrom any impmementation of the error interface so :), but well it's ok.

ifraixedes commented 6 years ago

yes, but other implementations of error aren't in this package. Disagreement isn't a problem, but I don't think that there is an strong argument to say that's really wrong.