-
Notifications
You must be signed in to change notification settings - Fork 399
Emit parsing errors as diagnostic records #1130
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
Changes from 9 commits
18c43e6
36061f5
edaca8e
37c095c
4c347a1
b3ef54a
8443abf
f6f3309
d34b8cb
0374851
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1526,23 +1526,27 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini | |||||
|
||||||
var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out List<DiagnosticRecord> diagnosticRecords); | ||||||
|
||||||
if (relevantParseErrors != null && relevantParseErrors.Count > 0) | ||||||
int emitParseErrors = severity == null ? 1 : severity.Count(item => item == "ParseError"); | ||||||
// Add parse errors first if requested! | ||||||
if ( relevantParseErrors != null && emitParseErrors == 1) | ||||||
|
if ( relevantParseErrors != null && emitParseErrors == 1) | |
if ( relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase))) |
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.
@rjmholt I made those changes
Outdated
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.
List<DiagnosticRecord> results = new List<DiagnosticRecord>(); | |
var results = new List<DiagnosticRecord>(); |
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.
fixed
Outdated
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.
foreach ( var parseError in relevantParseErrors ) | |
foreach (ParseError parseError in relevantParseErrors) |
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.
@rjmholt fixed
Outdated
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.
"" // no script file | |
string.Empty // no script file |
bergmeister marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
This regex might benefit from being a compiled private readonly static
field:
private readonly Regex s_aboutHelpRegex = new Regex("^about_.*help\\.txt$", RegexOptions.IgnoreCase | RegexOptions.Compiled);
Then here:
if ( Regex.Matches(Path.GetFileName(filePath), @"^about_.*help.txt$", RegexOptions.IgnoreCase).Count != 0) | |
if (s_aboutHelpRegex.IsMatch(Path.GetFileName(filePath))) |
EDIT: Put a backlash in for the .
before txt
to distinguish it from the regex wildcard.
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.
Or the other possibility is just to do:
string fileName = Path.GetFileName(filePath);
if (fileName.StartsWith("about_", StringComparison.OrdinalIgnoreCase)
&& fileName.EndsWith("help.txt", StringComparison.OrdinalIgnoreCase))
{
...
}
This is probably the most efficient option, since it doesn't even need to read the whole string.
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 went with the regex
Outdated
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.
var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords); | |
IEnumerable<ParseError> relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords); |
Outdated
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.
if ( relevantParseErrors != null && emitParseErrors == 1 ) | |
if (relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase))) |
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.
List<DiagnosticRecord> results = new List<DiagnosticRecord>(); | |
var results = new List<DiagnosticRecord>(); |
Outdated
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.
foreach ( var parseError in relevantParseErrors ) | |
foreach (ParseError parseError in relevantParseErrors) |
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.
Any reason you add all the results to a temp array and then use AddRange
instead of just adding directly to diagnosticRecords
?
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.
earlier debugging processes allowed me to see the new parser errors more easily
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.
converting this to a boolean and using
Any()
instead of.Count()
would be betterThere 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.
agreed!