Fix extents of UseSingularNoun and UseApprovedVerbs #495
Conversation
Fixes search logic to find the function name in a function definition. Prior to the fix, if the function definition started on the first line of a script, the extent would include the entire function definition. This commit fixes that bug.
| var funcNameToken = Tokens.Where( | ||
| token => ContainsExtent(functionDefinitionAst.Extent, token.Extent) | ||
| && token.Text.Equals(functionDefinitionAst.Name)); | ||
| if (funcNameToken.Any()) |
There was a problem hiding this comment.
You can simplify this a little bit by using funcNameToken.FirstOrDefault() which will return null if the enumerable has no items. Your logic is perfectly fine though, no need to change it. Just pointing out that this saves you from enumerating over the first item twice :)
There was a problem hiding this comment.
We return the extent property of the first element and not the first element itself. Assuming all the tokens are non-null, If we just use FirstOrDefault we still need to check for null (if no token was found) before getting the Extent property.
There was a problem hiding this comment.
Yeah, you'd need to check for null. The point is that FirstOrDefault saves you from two enumerations (which invoke a few methods internally). Any() causes one enumeration to see if there is a first item and First() causes another enumeration to get the first item. Like I said before, it's a very minor concern, I was just relaying a tip I've learned about IEnumerable over time.
There was a problem hiding this comment.
Ahh...now I see your point clearly. This is helpful to know. Thanks.
|
Thanks for the fast response to this bug! |
|
Reviewed 2 of 3 files at r1, 1 of 1 files at r2. Comments from Reviewable |
|
Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. Engine/Helper.cs, line 663 [r1] (raw file): Comments from Reviewable |
Fixes search logic to find the function name in a function definition.
Prior to the fix, if the function definition started on the first line of a script, the extent would include the entire function definition. This commit fixes that bug.
Fixes #492
This change is