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

[#760] Support JSON output in -compileonly #858

Merged
merged 5 commits into from Oct 16, 2019
Merged

Conversation

@jiujieti
Copy link
Contributor

jiujieti commented Oct 16, 2019

Added the support for -json flag and transform the errors into array JSON format.

)

// ErrorInJSON is the data structure for self-defined error
type ErrorInJSON struct {

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

This type does not need to be exported.

Suggest naming it errorWithContextInJSON.

The doc comment is too vague; suggested alternative:

// An auxiliary struct for converting errors with diagnostics information to JSON.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

will do

}

// SimplifiedErrorInJSON is the data structure for errors which contain limited info
type SimplifiedErrorInJSON struct {

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

This type does not need to be exported.

It can be named simpleErrorInJSON.

The doc comment is too vague; suggested alternative:

// An auxiliary struct for converting errors with only a message to JSON.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

will do

Message string `json:"message"`
}

// ErrorToJSON converts the error into JSON format

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

Please make doc comment full sentences.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

sure

var e interface{}
switch err := err.(type) {
case *eval.CompilationError:
e = []interface{}{ErrorInJSON{err.Context.Name, err.Context.Begin, err.Context.End, err.Message}}

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

Break into two lines.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

sure

case *eval.CompilationError:
e = []interface{}{ErrorInJSON{err.Context.Name, err.Context.Begin, err.Context.End, err.Message}}
case *parse.MultiError:
e = processMultiError(err)

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

You can inline the implementation of processMultiError here.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

I am definitely not the biggest fan of inlining all "simple" functions. Even though processMultiError is not called elsewhere, the function name could still help improve the readability.

Anyway here is just a loop, will do if you ask so

if er != nil {
return `[{"message":"Unable to convert the errors to JSON format"}]`
}
return string(jsonError)

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

There is no need to convert this to a string; returning a []byte is fine. You would need to convert the literal string above to a []byte though.

This comment has been minimized.

Copy link
@jiujieti

jiujieti Oct 16, 2019

Author Contributor

sure

}

func TestErrorsToJSON(t *testing.T) {
for i, test := range errorsJSONTests {

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

We have a library for table-based tests against functions; see

tt.Test(t, tt.Fn("ToString", ToString), tt.Table{
for an example.

Copy link
Member

xiaq left a comment

Mostly looks good now!

default:
e = []interface{}{simpleErrorInJSON{err.Error()}}
}
jsonError, er := json.Marshal(e)

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

Variables that keep errors are always called err or some name starting with err. Since err is taken in the function, call the second variable errMarshal.

}
jsonError, er := json.Marshal(e)
if er != nil {
return []byte(`[{"message":"Unable to convert the errors to JSON format"}]`)

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

"Unable to convert the errors to JSON" is sufficient, no need for "format"

}

// ErrorToJSON converts the error into JSON format.
func ErrorToJSON(err error) []byte {

This comment has been minimized.

Copy link
@xiaq

xiaq Oct 16, 2019

Member

Actually this function does not need to be exported either.

jiujieti added 2 commits Oct 16, 2019
@xiaq xiaq merged commit 4b7c69a into elves:master Oct 16, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@xiaq
Copy link
Member

xiaq commented Oct 16, 2019

Thank you! :D

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.