8000 WIP: Enable CA1507: Use nameof to express symbol names by xtqqczze · Pull Request #13875 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

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
@xtqqczze xtqqczze marked this pull request as ready for review October 26, 2020 21:32
{
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.

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.

3 participants

0