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

Shorten Some Commonly Used Method Names. #1075

Closed

Conversation

AdamSpeight2008
Copy link
Contributor

Implementation of Issue #999

Note: Incudes #1001 (This is where this branch originated)
Note: Builds with Test Failure ##1006

This function encapsulates a common coding pattern in the scanner
source. That of a `CanGetCharAtOffset( )` followed by multiple multiple
`PeekAheadChar( )` and comparison checks. As a result the scanner source
is a bit better to work with.
Renamed PeekAheadCHarAt -> Peek
CanGetCharAtOffset => CanGet(offset As Integer)
CanGetChar         => CanGet()
@gafter
Copy link
Member

gafter commented Mar 6, 2015

@AlekseyTs Do you find this change an improvement?

@gafter gafter self-assigned this Mar 6, 2015
@gafter gafter added this to the 1.0 (stable) milestone Mar 6, 2015
@AlekseyTs
Copy link
Contributor

I like the idea of renaming. There is one suggestion, "CanGet" should probably be "CanPeek".
It feels like the change itself introduces too much churn. I would prefer it to be straight rename, no reformatting of the code. For example, I do not find a change like the following (conversion to a single-line If) an improvement:

  •                If CanGetCharAtOffset(1) AndAlso BeginsBaseLiteral(PeekAheadChar(1)) Then
    
  •                    Return ScanNumericLiteral(precedingTrivia)
    
  •                End If
    
  •                If CanGet(1) AndAlso BeginsBaseLiteral(Peek(1)) Then Return ScanNumericLiteral(precedingTrivia)
    

Reformatting of conditions from multiple line to a single line fall into the same category, etc.

@gafter
Copy link
Member

gafter commented Mar 6, 2015

@AdamSpeight2008 Can you please update your pull request to address the comments from @AlekseyTs ?


InvalidIdentifier = True
End If
If Not IsIdentifierStartCharacter(ch) OrElse (IsConnectorPunctuation(ch) AndAlso Not (CanGet(Here + 1) AndAlso

Choose a reason for hiding this comment

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

Could you please keep the newline after the OrElse in the condition.
Now I need to scroll right the diff on GitHub to see the end of the proposed line.

If PeekAheadChar(Here) <> PeekAheadChar(FirstDateSeparator) Then
GoTo baddate
End If
If Peek(Here) <> Peek(FirstDateSeparator) Then GoTo baddate
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the formatting change here.

@gafter gafter added the Blocked label Mar 7, 2015
@AdamSpeight2008
Copy link
Contributor Author

I will not revert the formatting changes. You can submit a PR and I consider it,

@AdamSpeight2008
Copy link
Contributor Author

@KevinH-MS
Are you coming from a C# background? working with VB.
VB is predominately line orientated. Reading left to right to the end of the line.

IF ... Then ... you'll continue reading to the right.

If ... Then
...

You read L-R to the Then then move on to the next line. Maybe scanning forward to find the Else or `End If' to gauge the size of the block.

There is (currently) isn't any restriction on the width of the source. Also no font size has been specified, or screen width. etc, etc. I tend to make that limit 80chars. If the single line version will fit within that limit then I'll use it, otherwise use the multi line implementation.

@gafter
Copy link
Member

gafter commented Mar 15, 2015

@AdamSpeight2008 This PR contains both the desired renames and a number of unrelated formatting changes. We would prefer to have just the renames. You are most welcome to reopen the PR if it has been narrowed to those changes.

@gafter gafter closed this Mar 15, 2015
@KevinH-MS
Copy link
Contributor

@AdamSpeight2008
Quite the contrary, I've spent most of the past 10 years or so writing VB (and didn't really learn C# until ~2010). 😄
While you're certainly right in that VB/Basic are line/statement oriented languages, I would assert that since the advent of VB (early to mid 1990s), block ifs have been the norm (except for occasional use in argument validation). Part of this is because we haven't had a decent REPL for VB in years (but we're looking to change that). In any case, reasonable parties may disagree, and the above is mostly just my personal opinion. It is certainly the case, however, that block ifs are almost universally used across the Roslyn code base (so I would be disinclined to make changes unless they were broadly and consistently applied...and that should probably be a separate PR, as there is likely to be some debate...). Thanks

@AdamSpeight2008
Copy link
Contributor Author

@gafter #1627

@jaredpar jaredpar removed this from the 1.0 (stable) milestone Nov 23, 2015
@gafter gafter added the Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. label Dec 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Blocked Feature Request Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants