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

Remove internal errors for protolint to parse #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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