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

Display parsing errors in grammar compiler #4287

Conversation

pchaigno
Copy link
Contributor

@pchaigno pchaigno commented Oct 4, 2018

Display parsing errors in grammar compiler. Fixes #4141.

Here's how the error from #4141 would appear:

Checking docker is installed and running
$ docker ps
Registering new submodule: vendor/grammars/textmate-snort
$ git submodule add -f https://github.com/infosec-intern/textmate-snort vendor/grammars/textmate-snort
$ script/grammar-compiler add vendor/grammars/textmate-snort
  > The new grammar repository `vendor/grammars/textmate-snort` (from https://github.com/infosec-intern/textmate-snort) contains 1 errors:
  >     - Grammar conversion failed. File `syntaxes/snort.tmLanguage` failed to parse: XML syntax error on line 157: expected element name after <
  >
  > failed to compile the given grammar
Command failed. Aborting.

/cc @wesinator

Removed template as it doesn't apply.

@pchaigno pchaigno requested a review from vmg October 8, 2018 07:50
@@ -65,6 +65,9 @@ func (conv *Converter) tmpScopes() map[string]bool {
func (conv *Converter) AddGrammar(source string) error {
repo := conv.Load(source)
if len(repo.Files) == 0 {
if len(repo.Errors) > 0 {
return fmt.Errorf("Parsing error in '%s': %s", source, repo.Errors[0])
}
Copy link
Member

@lildude lildude Oct 9, 2018

Choose a reason for hiding this comment

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

This feels a bit odd to me: "We found no grammar files, but hey, here's an error."

I'm mostly AFK this week so don't have time to dig into this deeper myself, but couldn't we check for errors in the first if, eg:

if len(repo.Files) == 0 && len(repo.Errors) == 0 {
	return fmt.Errorf("source '%s' contains no grammar files", source)
}

... and only return if we really don't find any files or errors and leave the error reporting for the check slightly further down if we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right! That should work.

I'll update the pull request tonight.

Copy link
Contributor Author

@pchaigno pchaigno Oct 9, 2018

Choose a reason for hiding this comment

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

Done. I also updated the error message in the original post.

@pchaigno pchaigno force-pushed the grammar-compiler-display-parsing-errors branch from b78f398 to b6b6ef2 Compare October 9, 2018 19:00
@pchaigno
Copy link
Contributor Author

@lildude I think this is ready to be merged 😃

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

LGTM

@pchaigno pchaigno merged commit 58161f6 into github-linguist:master Oct 29, 2018
@pchaigno pchaigno deleted the grammar-compiler-display-parsing-errors branch October 29, 2018 11:44
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grammar-compiler XML parser errors are reported as "source contains no grammar files"
2 participants