make tidb adaptor in errors. by lysu · Pull Request #2 · pingcap/errors

lysu · 2018-09-05T07:32:30Z

What have you changed? (mandatory)

adaptor pkg/errors to tidb/tidb-operator, mainly worked by @gregwebs

ref issue

#1
https://github.com/lysu/errors/pull/2
pingcap/tidb-operator#80
pingcap/tidb#7151

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit tests
integration tests

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

no

Refer to a related PR or issue link (optional)

This change is

coocood · 2018-09-05T07:53:51Z

format_test.go

-		testGenericRecursive(t, err, want, wrappers, 3)
-	}
-}
+//func TestFormatGeneric(t *testing.T) {


Why comment out this test?

because we change the logic, N x N combination testcase is no longer suitable.

We can use a block comment /* */ to reduce the changed lines.

coocood · 2018-09-05T07:55:06Z

errors.go

 	}
 }

+type withStackAware interface {


Why not hasStackAware?

coocood · 2018-09-05T08:35:12Z

errors.go

 	hasStack() bool
 }

+func hasStack(err error) bool {


rename to errHasStack to avoid name conflict?

coocood · 2018-09-05T09:29:38Z

LGTM

This is done by using the new AddStack function instead of WithStack. There is also a StackError utility function exposed.

This is roughly (no generics) the Is function in the go 2 proposal

gregwebs · 2018-09-05T19:27:44Z

I have a PR to this branch: https://github.com/lysu/errors/pull/2

coocood · 2018-09-06T07:59:22Z

LGTM

lysu · 2018-09-06T08:19:06Z

I have no write access, need a merge~ then I will finish TiDB's PR, thx @coocood @gregwebs

gregwebs · 2018-09-06T08:20:58Z

I don't have write access.

coocood · 2018-09-06T09:05:56Z

@gregwebs @lysu
We need another LGTM to merge

zz-jason · 2018-09-06T09:13:10Z

errors.go

-//             StackTrace() errors.StackTrace
-//     }
-//
+// invoked. This information can be retrieved with the StackTracer interface that returns a StackTrace.


It seems that all the comment in this file is aligned to 80 columns, should we also keep the format?

zz-jason · 2018-09-06T09:15:09Z

errors.go

+// HasStack checks for this marker first.
+// Annotate/Wrap and Annotatef/Wrapf will produce this marker.
+type StackTraceAware interface {
+	hasStack() bool


should it be upper-case to HasStack?

zz-jason · 2018-09-06T09:25:54Z

group.go

+
+// WalkDeep does a depth-first traversal of all errors.
+// Any ErrorGroup is traversed (after going deep).
+// The visitor function can return false to end the traversal early


from the code logic, in order to terminate the traversal the visitor must return true

🤣 it's bug and never got to siblings, I try modify it and add a testcase. PTAL

zz-jason · 2018-09-06T09:28:59Z

group.go

+	}
+
+	// Go wide
+	if hasGroup, ok := err.(ErrorGroup); ok {


s/hasGroup/group/?

zhexuany

LGTM

zhexuany · 2018-09-06T14:50:27Z

Well, I don't have write access too.

gregwebs · 2018-09-06T15:11:44Z

group.go

-func WalkDeep(err error, visitor func(err error) bool) bool {
+// The visitor function can return true to end the traversal early
+// In that case, WalkDeep will return true, otherwise false.
+func WalkDeep(err error, visitor func(err error) bool) (done bool) {


I tend to avoid named parameters since new gophers are not necessarily familiar with them and they create stateful variables. I like the concept of trying to give a name to return types, it would just be nice if they didn't come with the extra ability. If keeping this variable I would also name this "early" rather than "done": from the visitor function perspective it is "done", but from the perspective of calling "WalkDeep", it is always done when it returns.

agree~ I rename them.

ps: I am not often using named parameters, too..but preivous days, I read this golang/go#20859 issue make me changed 🤣

Interesting

gregwebs · 2018-09-06T15:13:25Z

I did already approve the PR, which in my understanding implies "LGTM".

Thanks for the follow up fixes!

lysu · 2018-09-07T02:46:18Z

friendly ping @coocood

make tidb adaptor in errors.

859388f

coocood reviewed Sep 5, 2018

View reviewed changes

errors.go Outdated

}

}

type withStackAware interface {

Copy link

Member

coocood Sep 5, 2018

Choose a reason for hiding this comment

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

Why not hasStackAware?

address comment

d4948b3

coocood reviewed Sep 5, 2018

View reviewed changes

errors.go Outdated

hasStack() bool

}

func hasStack(err error) bool {

Copy link

Member

coocood Sep 5, 2018

Choose a reason for hiding this comment

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

rename to errHasStack to avoid name conflict?

lysu added 3 commits September 5, 2018 16:40

address comment

b6e6740

address comment

e7859cb

address comment

35194dc

lysu mentioned this pull request Sep 5, 2018

move from juju/errors to pkg/errors pingcap/tidb#7151

Merged

gregwebs added 9 commits September 5, 2018 09:31

remove logging

f28c952

don't change the pkg/errors interface

1953f98

some renames and use GetStackTracer

036cd12

avoid creating duplicate stack traces

2a9170a

This is done by using the new AddStack function instead of WithStack. There is also a StackError utility function exposed.

keep tests as pkg/errors

70a3293

fix tests

3397c8a

add an Unwrap function

831aa37

add the ErrorGroup interface

223d5a6

add a Find function for searching an error chain

e523106

This is roughly (no generics) the Is function in the go 2 proposal

lysu added 3 commits September 6, 2018 15:56

Merge pull request #1 from gregwebs/dev-tidb-adaptor-no-log

17afee1

Merge branch 'dev-tidb-adaptor' into dev-tidb-adaptor

0a67e46

Merge pull request #2 from gregwebs/dev-tidb-adaptor

2674428

gregwebs approved these changes Sep 6, 2018

View reviewed changes

zz-jason reviewed Sep 6, 2018

View reviewed changes

address comment

df8a356

zz-jason reviewed Sep 6, 2018

View reviewed changes

group.go Outdated

}

// Go wide

if hasGroup, ok := err.(ErrorGroup); ok {

Copy link

Member

zz-jason Sep 6, 2018

Choose a reason for hiding this comment

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

s/hasGroup/group/?

lysu added 2 commits September 6, 2018 18:04

address comment

ba07308

address comment

0485f1f

zhexuany approved these changes Sep 6, 2018

View reviewed changes

gregwebs reviewed Sep 6, 2018

View reviewed changes

address comment

e446394

coocood merged commit f295249 into pingcap:master Sep 7, 2018

SwanSpouse pushed a commit to SwanSpouse/errors that referenced this pull request Jul 14, 2019

make tidb adaptor in errors. (pingcap#2)

0fe4326

Conversation

lysu commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coocood commented Sep 5, 2018

Uh oh!

gregwebs commented Sep 5, 2018

Uh oh!

coocood commented Sep 6, 2018

Uh oh!

lysu commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregwebs commented Sep 6, 2018

Uh oh!

coocood commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhexuany left a comment

Choose a reason for hiding this comment

Uh oh!

zhexuany commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lysu Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregwebs commented Sep 6, 2018

Uh oh!

lysu commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lysu commented Sep 5, 2018 •

edited

Loading

lysu commented Sep 6, 2018 •

edited

Loading

lysu Sep 6, 2018 •

edited

Loading