Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Reenable compound assignment preference#17784

Merged
iSazonov merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Molkree:reenable-compound-assignmentCopy head branch name to clipboard
Jul 31, 2022
Merged

Reenable compound assignment preference#17784
iSazonov merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
Molkree:reenable-compound-assignmentCopy head branch name to clipboard

Conversation

@Molkree

@Molkree Molkree commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

PR Summary

Reenable compound assignment preference in .editorconfig.

PR Context

All violations of the rule should be fixed by now.
Fixes #17631

PR Checklist

@ghost ghost assigned TravisEz13 Jul 27, 2022
@Molkree

Molkree commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

Okay, I already see some missed cases. I wonder what is different between the CI and my local environment?

@Molkree

Molkree commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

Hm, one of them looks like a false positive. In /src/System.Management.Automation/singleshell/config/MshSnapinInfo.cs(1287,25):

                        if (s_defaultMshSnapins == null)
                        {
                            s_defaultMshSnapins = new List<DefaultPSSnapInInformation>()
                            {
#if !UNIX
                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Diagnostics", "Microsoft.PowerShell.Commands.Diagnostics", null,
                                    "GetEventResources,Description", "GetEventResources,Vendor"),
#endif
                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Host", "Microsoft.PowerShell.ConsoleHost", null,
                                    "HostMshSnapInResources,Description", "HostMshSnapInResources,Vendor"),

                                s_coreSnapin,

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Utility", "Microsoft.PowerShell.Commands.Utility", null,
                                    "UtilityMshSnapInResources,Description", "UtilityMshSnapInResources,Vendor"),

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Management", "Microsoft.PowerShell.Commands.Management", null,
                                    "ManagementMshSnapInResources,Description", "ManagementMshSnapInResources,Vendor"),

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Security", "Microsoft.PowerShell.Security", null,
                                    "SecurityMshSnapInResources,Description", "SecurityMshSnapInResources,Vendor")
                            };

#if !UNIX
                            if (!Utils.IsWinPEHost())
                            {
                                s_defaultMshSnapins.Add(new DefaultPSSnapInInformation("Microsoft.WSMan.Management", "Microsoft.WSMan.Management", null,
                                    "WsManResources,Description", "WsManResources,Vendor"));
                            }
#endif
                        }

It's not just an assignment inside the outer if, so analyzer shouldn't trigger here, what do you think?

@Molkree

Molkree commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

I wonder what is different between the CI and my local environment?

The system is different, CI errored on Linux and macOS and I use Windows. 3 cases were inside #if UNIX hence why they didn't trigger for me.

4th one has #if !UNIX clause inside hence why it doesn't trigger on my local machine. But in CI Analyzer thinks there's no extra code inside and suggests this change. I'm not sure if it is correct behaviour.

@Molkree

Molkree commented Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

It's not just an assignment inside the outer if, so analyzer shouldn't trigger here, what do you think?

We can rewrite the code block into this and it should please Analyzer on both platforms:

#if UNIX
                        s_defaultMshSnapins ??= new List<DefaultPSSnapInInformation>()
                        {
                            new DefaultPSSnapInInformation("Microsoft.PowerShell.Host", "Microsoft.PowerShell.ConsoleHost", null,
                                "HostMshSnapInResources,Description", "HostMshSnapInResources,Vendor"),

                            s_coreSnapin,

                            new DefaultPSSnapInInformation("Microsoft.PowerShell.Utility", "Microsoft.PowerShell.Commands.Utility", null,
                                "UtilityMshSnapInResources,Description", "UtilityMshSnapInResources,Vendor"),

                            new DefaultPSSnapInInformation("Microsoft.PowerShell.Management", "Microsoft.PowerShell.Commands.Management", null,
                                "ManagementMshSnapInResources,Description", "ManagementMshSnapInResources,Vendor"),

                            new DefaultPSSnapInInformation("Microsoft.PowerShell.Security", "Microsoft.PowerShell.Security", null,
                                "SecurityMshSnapInResources,Description", "SecurityMshSnapInResources,Vendor")
                        };
#else
                        if (s_defaultMshSnapins == null)
                        {
                            s_defaultMshSnapins = new List<DefaultPSSnapInInformation>()
                            {
                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Diagnostics", "Microsoft.PowerShell.Commands.Diagnostics", null,
                                    "GetEventResources,Description", "GetEventResources,Vendor"),
                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Host", "Microsoft.PowerShell.ConsoleHost", null,
                                    "HostMshSnapInResources,Description", "HostMshSnapInResources,Vendor"),

                                s_coreSnapin,

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Utility", "Microsoft.PowerShell.Commands.Utility", null,
                                    "UtilityMshSnapInResources,Description", "UtilityMshSnapInResources,Vendor"),

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Management", "Microsoft.PowerShell.Commands.Management", null,
                                    "ManagementMshSnapInResources,Description", "ManagementMshSnapInResources,Vendor"),

                                new DefaultPSSnapInInformation("Microsoft.PowerShell.Security", "Microsoft.PowerShell.Security", null,
                                    "SecurityMshSnapInResources,Description", "SecurityMshSnapInResources,Vendor")
                            };

                            if (!Utils.IsWinPEHost())
                            {
                                s_defaultMshSnapins.Add(new DefaultPSSnapInInformation("Microsoft.WSMan.Management", "Microsoft.WSMan.Management", null,
                                    "WsManResources,Description", "WsManResources,Vendor"));
                            }
                        }
#endif

Or we can ignore this rule locally.

I'll let the maintainers decide.

@iSazonov

Copy link
Copy Markdown
Collaborator

Hm, one of them looks like a false positive. In /src/System.Management.Automation/singleshell/config/MshSnapinInfo.cs(1287,25):

Please suppress locally.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Jul 28, 2022
Comment thread .editorconfig Outdated
@Molkree

Molkree commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

Please suppress locally.

@iSazonov, done

@iSazonov

Copy link
Copy Markdown
Collaborator

@Molkree I guess CIs fail because we need to fix compound assignment in test code (weblistener).

@Molkree

Molkree commented Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

@iSazonov, I am stumped, from failing tests I can only see that the WebListener did not start before the timeout was reached., with no pointers as to why. Any help here?

@iSazonov

Copy link
Copy Markdown
Collaborator

@Molkree Try compile test\tools\WebListener\ - I guess there are compound assignments we should fix.

@Molkree

Molkree commented Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

Try compile test\tools\WebListener\

@iSazonov, thank you!

@iSazonov iSazonov assigned iSazonov and unassigned TravisEz13 Jul 31, 2022
@iSazonov iSazonov merged commit 7f69d74 into PowerShell:master Jul 31, 2022
@Molkree Molkree deleted the reenable-compound-assignment branch July 31, 2022 13:56
@ghost

ghost commented Aug 11, 2022

Copy link
Copy Markdown

🎉v7.3.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Sep 29, 2022
@TravisEz13 TravisEz13 mentioned this pull request Sep 30, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix IDE0074 - 'Use compound assignment' as reported by new style rules

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.