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

Replace fmt.Errorf with errors.Errorf #3627

Merged
merged 4 commits into from
Jul 11, 2019
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jul 3, 2019

Fixes #3449


This change is Reviewable

@martinmr martinmr requested review from gitlw, manishrjain and a team as code owners July 3, 2019 18:21
@martinmr martinmr requested review from campoy, danielmai and a team and removed request for a team, gitlw and manishrjain July 3, 2019 18:21
campoy
campoy previously requested changes Jul 3, 2019
Copy link
Contributor

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 50 files at r1.
Reviewable status: 5 of 50 files reviewed, 3 unresolved discussions (waiting on @campoy, @danielmai, and @martinmr)


conn/pool.go, line 39 at r2 (raw file):

	ErrNoConnection = errors.Errorf("No connection exists")
	// ErrUnhealthyConnection indicates the connection to a node is unhealthy.
	ErrUnhealthyConnection = errors.Errorf("Unhealthy connection")

These should be errors.New technically ... but I don't think it matters much, tbh


dgraph/cmd/alpha/http.go, line 108 at r2 (raw file):

	uintVal, err := strconv.ParseUint(value, 0, 64)
	if err != nil {
		return 0, errors.Errorf("Error: %+v while parsing %s as uint64", err, name)

These should be replaced with errors.Wrapf(err, "could not parse %q as unit64", name)

And same for all the other fmt.Errorf which embed an error.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 50 files reviewed, 3 unresolved discussions (waiting on @campoy, @danielmai, and @golangcibot)


conn/pool.go, line 39 at r2 (raw file):

Previously, campoy (Francesc Campoy) wrote…

These should be errors.New technically ... but I don't think it matters much, tbh

Just changed this since they are typed errors. I left the rest as-is.


dgraph/cmd/alpha/http.go, line 108 at r2 (raw file):

Previously, campoy (Francesc Campoy) wrote…

These should be replaced with errors.Wrapf(err, "could not parse %q as unit64", name)

And same for all the other fmt.Errorf which embed an error.

Done.


dgraph/cmd/migrate/table_guide.go, line 24 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)


Done.

@martinmr martinmr requested a review from campoy July 3, 2019 23:00
@martinmr martinmr dismissed campoy’s stale review July 9, 2019 18:30

addressed comments

Copy link
Contributor

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Reviewed 44 of 50 files at r1, 1 of 1 files at r2, 22 of 22 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @campoy, @danielmai, and @golangcibot)

@martinmr martinmr merged commit 0c68c62 into master Jul 11, 2019
@martinmr martinmr deleted the martinmr/use-errors-errorf branch July 11, 2019 01:08
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix calls to fmt.Errorf and errors.Errorf
3 participants