Navigation Menu

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

Use NameOf instead of explicit string literals. #1879

Closed
wants to merge 6 commits into from

Conversation

AdamSpeight2008
Copy link
Contributor

Change to use VB.net's NameOf operator, instead of explicit strings.

@@ -195,7 +195,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
Throw New ArgumentException("Must resolve overloads on PropertySymbol or MethodSymbol", "TMember")
End If
If isProperties And Not typeArguments.IsEmpty Then
Throw New ArgumentException(VBResources.PropertiesCanNotHaveTypeArguments, "typeArguments")
Throw New ArgumentException(VBResources.PropertiesCanNotHaveTypeArguments, NameOf(members))
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect.

@gafter
Copy link
Member

gafter commented Apr 9, 2015

Good idea, but bad execution. No thanks.

@gafter gafter closed this Apr 9, 2015
@AdamSpeight2008
Copy link
Contributor Author

@gafter I've pushed an update correcting the error introduce by the tool I used. I've gone through each one and made sure that they are all correct.
Also filed a bug / issue on that tool's page.

@sharwell
Copy link
Member

@gafter Considering how easy it is to write automated transforms using Roslyn, it's a pretty safe bet that if you see obvious errors in something otherwise simple like this you've found a bug in someone's tools. 👍

@AdamSpeight2008
Copy link
Contributor Author

@gafter @sharwell I should have at least checked the diff. 🔍

@gafter
Copy link
Member

gafter commented Apr 13, 2015

Looks good to me. @agocke does this look good to you?

@gafter gafter added this to the 1.0 (stable) milestone Apr 13, 2015
@gafter gafter added the 4 - In Review A fix for the issue is submitted for review. label Apr 13, 2015
@@ -1,5 +1,4 @@
' Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need the copyright notice.

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 can't remember changing that but will fix it.

@agocke
Copy link
Member

agocke commented Apr 13, 2015

Barring the missing copyright notice, LGTM

@gafter
Copy link
Member

gafter commented Apr 15, 2015

Integrated in 8b65942

@gafter gafter closed this Apr 15, 2015
@gafter gafter removed the 4 - In Review A fix for the issue is submitted for review. label Apr 15, 2015
@AdamSpeight2008 AdamSpeight2008 mentioned this pull request Apr 15, 2015
End If

Dim vbsymbol = symbol.EnsureVbSymbolOrNothing(Of symbol)("symbol")
Dim vbsymbol = symbol.EnsureVbSymbolOrNothing(Of symbol)(NameOf(position))
Copy link

Choose a reason for hiding this comment

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

@AdamSpeight2008 @gafter This appears to have been accidentally changed from symbol to position.

@AdamSpeight2008
Copy link
Contributor Author

@mikedn @gafter Reopen and I'll push the changes.
The tool was using the first parameter rather than using the correct parameter.

I did manually check the diff.

@gafter
Copy link
Member

gafter commented Apr 16, 2015

Integrated in 8b65942

@gafter
Copy link
Member

gafter commented Apr 16, 2015

@mikedn Thanks for catching those! I'll fix them immediately.

@AdamSpeight2008
Copy link
Contributor Author

@gafter See #2018 for the fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants