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

add existing row to unique constraint failure #380

Merged
merged 5 commits into from Apr 27, 2021
Merged

Conversation

bheni
Copy link
Contributor

@bheni bheni commented Apr 22, 2021

No description provided.

@bheni bheni marked this pull request as draft April 22, 2021 00:27
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Not a big fan of reintroducing the ExpectedErrStr, but it's fine for now.

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

Bikeshedding!

@@ -912,6 +912,7 @@ func TestScriptWithEngine(t *testing.T, e *sqle.Engine, harness Harness, script
Query: script.Query,
Expected: script.Expected,
ExpectedErr: script.ExpectedErr,
ExpectedErrStr: script.ExpectedErrStr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, gofmt.

@@ -265,3 +260,46 @@ func CastSQLError(err error) (*mysql.SQLError, bool) {

return mysql.NewSQLError(code, sqlState, err.Error()), false
}

type UniqueKeyError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. We can't make this look more like https://blog.golang.org/go1.13-errors ? If we're going to be adding test methods, etc.

Usage like

errors.Is(err, &UniqueKeyError{})

or

errors.Is(err, &UniqueKeyError{IsPK: true})

with

var ue *UniqueKeyError
if errors.As(err &ue) {
    if ue.IsPK {
        ...
    } else {
        ...
    }
}

all looking reasonable. Or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reltuk I don't really want to be pulling in two packages named errors with incompatible functionality. In gms we use the "gopkg.in/src-d/go-errors.v1" instead of the standard package "errors".

An example incompatibility is go-errors packages define a Wrap method which isn't compatible with Unwrap defined in the standard errors package.

I did rework this so it looks more like the rest of the error handling in this repo. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

@bheni we should sync up on this. I have another branch that's trying to remove these error definitions and use ErrDuplicateKey only

@reltuk
Copy link
Contributor

reltuk commented Apr 23, 2021

Cherry-picked these test changes here: #384

@bheni
Copy link
Contributor Author

bheni commented Apr 23, 2021

This bike shed is going to be so dope

@@ -265,3 +260,46 @@ func CastSQLError(err error) (*mysql.SQLError, bool) {

return mysql.NewSQLError(code, sqlState, err.Error()), false
}

type UniqueKeyError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

@bheni bheni marked this pull request as ready for review April 26, 2021 17:20
@bheni bheni merged commit 4c27ceb into master Apr 27, 2021
@bheni bheni deleted the bh/unique-error branch April 27, 2021 17:06
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.

None yet

5 participants