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

WIP: Enable CA1507: Use nameof to express symbol names #13875

Closed
wants to merge 1 commit into from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Oct 25, 2020

@ghost ghost assigned daxian-dbw Oct 25, 2020
@xtqqczze xtqqczze mentioned this pull request Oct 25, 2020
8 tasks
@@ -185,7 +185,7 @@ private bool FilterMatch(ManagementObject obj)
foreach (string desc in Description)
{
WildcardPattern wildcardpattern = WildcardPattern.Get(desc, WildcardOptions.IgnoreCase);
if (wildcardpattern.IsMatch((string)obj["Description"]))
if (wildcardpattern.IsMatch((string)obj[nameof(Description)]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here Description is a property name of ManagementObject.
Or can it be:

Suggested change
if (wildcardpattern.IsMatch((string)obj[nameof(Description)]))
if (wildcardpattern.IsMatch((string)obj[nameof(obj.Description)]))

Also what about "InstalledBy" in the file?

I suggest to revert such non-obvious changes here and below because we can break something tricky.

@@ -89,55 +89,55 @@ internal bool HasHostWindow
{
get
{
return (bool)_graphicalHostReflectionWrapper.GetPropertyValue("HasHostWindow");
return (bool)_graphicalHostReflectionWrapper.GetPropertyValue(nameof(HasHostWindow));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same. HasHostWindow is a property of GraphicalHostReflectionWrapper.

I see the same patterns below.

RemotingEncoder.AddNoteProperty<object>(dest, "TargetObject", delegate () { return TargetObject; });
RemotingEncoder.AddNoteProperty<string>(dest, "FullyQualifiedErrorId", delegate () { return FullyQualifiedErrorId; });
RemotingEncoder.AddNoteProperty<InvocationInfo>(dest, "InvocationInfo", delegate () { return InvocationInfo; });
RemotingEncoder.AddNoteProperty<Exception>(dest, nameof(Exception), delegate () { return Exception; });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, is the reference to Exception right?
Below in the file too.

informationRecord.ProcessId = RemotingDecoder.GetPropertyValue<uint>(inputObject, "ProcessId");
informationRecord.NativeThreadId = RemotingDecoder.GetPropertyValue<uint>(inputObject, "NativeThreadId");
informationRecord.ManagedThreadId = RemotingDecoder.GetPropertyValue<uint>(inputObject, "ManagedThreadId");
informationRecord.User = RemotingDecoder.GetPropertyValue<string>(inputObject, nameof(User));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same.

@xtqqczze xtqqczze changed the title Enable CA1507: Use nameof to express symbol names WIP: Enable CA1507: Use nameof to express symbol names Oct 29, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2020

@xtqqczze Please address my comments.

@ghost ghost added the Review - Needed The PR is being reviewed label Nov 12, 2020
@ghost
Copy link

ghost commented Nov 12, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@xtqqczze xtqqczze marked this pull request as draft November 13, 2020 01:05
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 16, 2020
@iSazonov
Copy link
Collaborator

@xtqqczze Can we continue? If there are doubts please revert and suppress locally.

@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 21, 2020
@ghost ghost added the Stale label Dec 6, 2020
@ghost
Copy link

ghost commented Dec 6, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants