Add defined errors to the package

[ghostlandr]

Hello! I have finally come back for #29 and solved it basically the way you suggested. I'm looking to try to get involved with more Go OSS and this is my first PR so far.

My biggest concern is that it's theoretically a breaking change, as if someone was depending on having error details in Error previously, those are now gone.

The reason this is so many changes is because as part of adding the ErrorDetails to Root I also changed the usages of Root to use labelled properties rather than specifying everything in each initialization.

Let me know if I should add an example to either the Readme or to the examples folder of this in use?

[anaskhan96]

Hi @gdholtslander

First of all, thank you for the PR. This is amazing! Breaking changes are okay, as this will be a separate release.

I do have an issue though.

• Not sure if we should be including two error related fields in the Root struct. That could lead to two possible nil pointer dereference exceptions by a programmer instead of one on every success.
• On top of that, there's actually a duplication of error explanation text taking place. Let's take ErrElementNotFound for example. It has two strings, on in Root.Error.Error() which is "element not found", and one in Root.ErrorDetails which is "element x with attributes y not found"

Either we dispose of the ErrorDetails, or if we really want to give an explanation from our side, we have Root.Error be a variable of our custom composite type of error, something like this

type SoupError struct {
    Type ErrorType // our enum with values `ErrUnableToParse`, `ErrElementNotFound`, etc.
    error
}

This way, we can get the type using Root.Error.Type and the detailed text using Root.Error.Error()

Let me know what you think.

Oh, and also unit test cases. Very important for an integral change like this :P

[ghostlandr]

I like the second approach! I had that one essentially coded up but I was worried about backwards compatibility - essentially that people would already be checking for certain error strings and those wouldn't be there anymore. If it's still 1.x then there shouldn't be breaking changes... unless you meant we'd release it as a 2.0 if we added this. A bit extreme just for errors! 😂

I think if we implement Error() as the old message, it should still be backward compatible...

I'll keep working on this. Thanks for the feedback!

[ghostlandr]

I didn't realize it has been almost a year since I made the issue. Whoops 😨

[ghostlandr]

Second approach we talked about is implemented, and I added some tests. I also added an example of some error handling one could do, but it's quite basic.

Looking forward to hearing what you think about this now.

[anaskhan96]

Merging this, thank you again for the PR! :)