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.

Sign up for GitHub

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

Jump to bottom

Unification of the contents of TryScanToken and ScanTokenFullWidth. #1381

Closed
wants to merge 12 commits into from
Closed

Unification of the contents of TryScanToken and ScanTokenFullWidth. #1381

wants to merge 12 commits into from

Conversation

AdamSpeight2008
Copy link
Contributor

  • Small Refactoring with the unified method.
    • Use a common lengthWithMeyBeEquals
    • Change section to use IF( , ) instead.

@AdamSpeight2008
Copy link
Contributor Author

Also note that on my local machine I get the following single failure.


Assert.Equal() Failure
Position: First difference is at position 0
Expected: My Resource 1 ENU string
Actual:   

   at Microsoft.CodeAnalysis.UnitTests.Diagnostics.DiagnosticLocalizationTests.TestDiagnosticLocalization() in Src\Compilers\Core\CodeAnalysisTest\Diagnostics\DiagnosticLocalizationTests.cs:line 70

VladimirReshetnikov
VladimirReshetnikov reviewed Mar 19, 2015
View reviewed changes
src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb
End If

Dim ch As Char = PeekChar()
Private Function _Unified_(precedingTrivia As SyntaxList(Of VisualBasicSyntaxNode), ch As Char, fullWidth As Boolean) As SyntaxToken
Copy link

Choose a reason for hiding this comment

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

Are you going to rename this method eventually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am open to suggestions for the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VladimirReshetnikov
Current thoughts are on the lines of

  1. ScanToken_Common
  2. ScamTokenIternals

@gafter gafter added this to the 1.0 (stable) milestone Mar 19, 2015
@AdamSpeight2008 AdamSpeight2008 deleted the Scanner(Unification) branch March 20, 2015 19:09
@AdamSpeight2008 AdamSpeight2008 mentioned this pull request Mar 20, 2015
Closed
@gafter gafter removed this from the 1.0 (stable) milestone Mar 20, 2015
@AdamSpeight2008
Copy link
Contributor Author

@gafter
My remote repo's branch was different to my local one, and the git commands I used screw this up.
I'm unable to reopen do this The Scanner(Unification) was force pushed or recreated.
Suggestions?

Recreated Branch

@jaredpar
Copy link
Member

@AdamSpeight2008 the following set of commands will take the contents of Scanner_Unify (PR #1427) and move them to Scanner(Unification) (this PR). Note:

  • This set of commands assumes that the remote origin points to https://github.com/AdamSpeight2008/roslyn
  • This will overwrite the contents of Scanner(Unification). If you want to keep this around you should save it in another branch before executing these commands.

That being said here is the code to move the new code into this PR.

First step is to switch the branch to Scanner(Unification) which is the basis of this PR.

git checkout Scanner(Unification)

Next we want this branch to have the same contents as Scanner_Unify.

git reset --hard Scanner_Unify

Now we want to push this up to GitHub so that this PR will update.

git push origin Scanner(Unification):Scanner(Unification) -force

The use of -force is necessary here because these branches don't have a shared history (at least that I can see).

@AdamSpeight2008
Copy link
Contributor Author

@jaredpar
result

error: dst ref refs/heads/Scanner(Unification) receives from more than one src.
error: failed to push some refs to 'https://github.com/AdamSpeight2008/roslyn-AdamSpeight2008.git'

@AdamSpeight2008
Copy link
Contributor Author

@jaredpar I think I fix it now.

@AdamSpeight2008
Copy link
Contributor Author

@jaredpar I been in contact with GitHub on how to fix this issue.

Ivan Žužak support@github.com

Sorry, Adam -- we can't do anything about that pull request after you force pushed and deleted the head branch once. You'll need to open a new pull request.

Let us know if you have any other questions.

Cheers,
Ivan

Please consider #1463 as the fix. I'll not touch that branch at all

@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
Reviewers
No reviews
Assignees
No one assigned
Labels
Area-Compilers 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AdamSpeight2008 @jaredpar @VladimirReshetnikov @gafter @theoy @dnfclas