Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLIN-2317 Add related field to foreign key error #84

Merged
merged 2 commits into from
Sep 12, 2019

Conversation

mochacat
Copy link
Contributor

Changes required by skuid/warden#326

errors.go Outdated
}

// IsForeignKeyError indicates that the error is a foreign key
func (e *ForeignKeyError) IsForeignKeyError() (bool, string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to not have this function. I have two issues with it.

  1. In my opinion, a function myType.isMyType() that always returns true doesn't make much sense to me. Go already provides a way to check whether or not an interface is a certain type.

  2. It returns a secondary result that is not covered in the name of the function at all. I would rather have a separate function called ForeignKeyError.GetKey() that returned the key.

I think I disagree with Dave Cheney about asserting an error's behavior instead of its type. To me his examples fall flat because you still have to know about the package you're using if you want to use its functionality in the first place. I would rather just do type checking on the errors than create an interface that the error must implement. I guess that's just my opinion, we can talk more about it later if you would like.

Copy link
Contributor Author

@mochacat mochacat Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you are coming for IsForeignKeyError. Now that I think of it more I don't think IsForeignKeyError is necessary, because all we need to do in warden is this:
fErr, ok := err.(ForeignKeyError)

ForeignKeyError being the interface we have defined in warden. I'm already doing that in SplitKeys & GetFieldType in errors/database.go in warden.

If this were a public package, I'd slightly prefer the caller defining the error contract by behavior (if they wanted to) in case things change. Since we are using this as an internal package for ourselves and we trust ourselves (I think??), I guess it isn't a big deal. That's also just my opinion, so I don't mind going back to asserting by type.

I don't think I can complain too much about this since there will also be unwrapping in Go 1.13 when we upgrade too.

err1 := &myError{}
err2 := fmt.Errorf(“%w”, err1)
errors.Unwrap(err2) == err1
errors.Is(err2, err1) == true
var me myError
errors.As(err2, &me) == true

@mochacat mochacat merged commit f7812bf into master Sep 12, 2019
@mochacat mochacat deleted the PLIN-2317-foreign-key-error-related-field branch September 12, 2019 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants