-
Couldn't load subscription status.
- Fork 315
Rubberduck.vb editor c#6&7 update #3610
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
Rubberduck.vb editor c#6&7 update #3610
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.
Mostly good. One nit and 2 suggestions that requires others' opinions.
| } | ||
|
|
||
| public string ApplicationName { get { return "(unknown)"; } } | ||
| public string ApplicationName => "(unknown)"; |
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 wonder if this should be localized?
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 is probably used in code, see HostApplicationBase#L20 Also AFAICT this is not user-facing
| if (IsWrappingNullReference) return string.Empty; | ||
| vbext_ProcKind procKind; | ||
| return Target.get_ProcOfLine(line, out procKind); | ||
| return IsWrappingNullReference ? string.Empty : Target.get_ProcOfLine(line, out _); |
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 is beyond the scope of this PR but I should point out that this method basically discards the vbext_ProcKind that's returned. Is there any value in returning this as an optional out parameter back to the callees of the GetProcOfLine, making it their choice whether they care about it or not?
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.
@bclothier there would if the caller used the VBIDE API, but the VBIDE API wrapper types are rather deep, ...it's possible that future calls to GetProcOfLine might possibly need the parameter, but somewhat unlikely. IOW if it's not needed by now... YAGNI ;-)
| public int EndColumn { get; } | ||
|
|
||
| public int LineCount { get { return _endLine - _startLine + 1; } } | ||
| public int LineCount => EndLine - StartLine + 1; |
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 really don't like expressions like those. I would much prefer that it's (EndLine - StartLine) + 1. Yes I know it may be considered redundant but in this case, it reduces the cognition load. (and yes I'm the dolt who didn't follow his own advice)
No description provided.