-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[release/v1.0] Include line and column numbers in lexer errors. #3772
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
@martinmr you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good idea to encapsulate struct creation in the NewLexer(mutation string)
method it will enable future change to be handled with greater flexibility for the library authors.
Starting at lex/lexer.goL92:95
and again at lex/lexer.goL131:134
create the same Item
struct. Would it be possible to create a defaultItem
as a package var so that only need to change things in one place?
Reviewed with ❤️ by PullRequest
@@ -144,7 +184,10 @@ func (l *Lexer) Run(f StateFn) *Lexer { | |||
func (l *Lexer) Errorf(format string, args ...interface{}) StateFn { | |||
l.items = append(l.items, Item{ | |||
Typ: ItemError, | |||
Val: fmt.Sprintf("while lexing %v: "+format, append([]interface{}{l.Input}, args...)...), | |||
Val: fmt.Sprintf("while lexing %v at line %d column %d: "+format, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps create a lexerErrorFormat
that combines format
with the updated fmt string?
@@ -268,6 +328,11 @@ func (l *Lexer) IsEscChar(r rune) bool { | |||
return false | |||
} | |||
|
|||
// IsEndOfLine returns true if the rune is a Linefeed or a Carriage return. | |||
func IsEndOfLine(r rune) bool { | |||
return r == '\u000A' || r == '\u000D' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding these values as constants of the lex
package as other consumers may want to inspect against the same criteria. This will make it easier to share.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @gitlw, @manishrjain, and @martinmr)
This PR cherry-picks 2129101 to support line
and column numbers in lexer errors. It also changes parse_mutation.go to use them.
This change is