Skip to content

Conversation

@PaulSonOfLars
Copy link
Contributor

@PaulSonOfLars PaulSonOfLars commented Nov 29, 2019

Certain internal errors get thrown on parse issues, which makes it impossible to open a switch statement on them.

This PR removes those, instead returning the latest error, which seems to be the most relevant from the parse tests I ran.

Sample protolint output from previous errors:
image

As you can see, both errors are very similar. In my opinion, the second one is slightly more helpful; hence why i return that one. They are both guaranteed to be non-nil given the previous if statements.

@yoheimuta yoheimuta self-requested a review December 3, 2019 00:58
Copy link
Owner

@yoheimuta yoheimuta left a comment

Choose a reason for hiding this comment

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

Thank you for your improvement!
I added a small comment.

parseEnumFieldErr: enumFieldErr,
parseEmptyStatementErr: emptyErr,
}
return nil, nil, scanner.Position{}, emptyErr
Copy link
Owner

Choose a reason for hiding this comment

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

Certain internal errors get thrown on parse issues, which makes it impossible to open a switch statement on them.

I understand your need.

This PR removes those, instead returning the latest error, which seems to be the most relevant from the parse tests I ran.
As you can see, both errors are very similar. In my opinion, the second one is slightly more helpful; hence why i return that one.

I thought this composite type was necessary to include enough errors so that end-users(or rather, maintainers) can see what input token is being expected.

In terms of usability, the error message should have looked like below.

func (e *parseEnumBodyStatementErr) Error() string {
	return fmt.Sprintf(
		"expected enumField or emptyStatement and tried parsing each, "+
			"but following two errors cropped up: (1) parseEnumFieldErr`%v` (2) parseEmptyStatementErr`%v`",
		e.parseEnumFieldErr,
		e.parseEmptyStatementErr,
	)
}

expected enumField or emptyStatement and tried parsing each, but following two errors cropped up: (1) parseEnumFieldErrfound "\"option\"(Token=2, Pos=simple.proto:9:5)" but expected [=] at /GOPATH/src/github.com/yoheimuta/go-protoparser/parser/enum.go:242 (2) parseEmptyStatementErrfound "option" but expected [;]

makes it impossible to open a switch statement on them.

Is it not enough to prepare a method/interface for extracting inner errors?

type CompositeError interface {
	Errors() []error
}
func (e *parseEnumBodyStatementErr) Errors() []error {
	return []error{e.parseEnumFieldErr, e.parseEmptyStatementErr}
}

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.

2 participants