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

[#873] fix json output of -compileonly #874

Merged
merged 1 commit into from Dec 9, 2019
Merged

Conversation

@sblundy
Copy link
Contributor

sblundy commented Dec 4, 2019

The errorToJSON() method checks whether the error returned is a *parse.MultiError. However, rather than a pointer to a MultiError, an instance was being passed in.

Updated the the instance methods of MultiError to take pointers as receivers to be in line with what was expected. This further required updating some other references.

The errorToJSON() method in shell/errors_to_json.go checks whether the error returned is a *parse.MultiError. However, rather than a pointer to a MultiError, an instance was being passed in. Updated the the instance methods of MultiError to take pointers as receivers to be in line with what was expected. This further required updating some other references.
@xiaq
Copy link
Member

xiaq commented Dec 4, 2019

Hmm, what about just changing the type switch in errotToJSON?

@sblundy
Copy link
Contributor Author

sblundy commented Dec 5, 2019

Hmm, what about just changing the type switch in errotToJSON?

Consistency mostly. Pointer receivers are the norm, instance ones only when necessary. MultiError had a mix of both. Unless there's a reason that I missed, inconsistency tends to foster bugs.

Thinking about it more, the most idiomatic way would be with an interface that both eval.CompilationError and parse.MultiError would implement, but that'd be an even bigger change.

Something like this:

func errorToJSON(err error) []byte {
	var e interface{}
	type hasParseErrors interface {
		ParseErrors() []*parse.Error
	}
	switch err := err.(type) {
	case hasParseErrors:
		var errArr []errorInJSON
		for _, v := range err.ParseErrors() {
			errArr = append(errArr,
				errorInJSON{v.Context.Name, v.Context.Begin, v.Context.End, v.Message})
		}
		e = errArr
	default:
		e = []interface{}{simpleErrorInJSON{err.Error()}}
	}
	jsonError, errMarshal := json.Marshal(e)
	if errMarshal != nil {
		return []byte(`[{"message":"Unable to convert the errors to JSON"}]`)
	}
	return jsonError
}
@xiaq
Copy link
Member

xiaq commented Dec 9, 2019

Ah, this type has methods of both value and pointer receivers. I agree that that is bad and your PR is in the correct direction, which I will merge.

But I am not convinced on the larger point that that pointer receivers are the norm; I don't think there is such consensus in the Go community. Value receivers are useful from time to time.

Regarding how to refactor this further: eval.CompilationError and parse.Error have the same fields, just slightly different implementations of how they print themselves, because they have different types. We can consolidate the difference by introducing a more general Error type with an extra "Type" field. The diag package is a good place for this new Error type.

@xiaq xiaq merged commit d076f36 into elves:master Dec 9, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.