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

Allow formatter to optionally correct aliases #1277

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Jun 30, 2019

PR Summary

Invoke-Formatter has a hard-coded list of rules that is supports. Add support for PSAvoidUsingCmdletAliases as the community has expressed interest in being able to auto-replace them when formatting in VS-Code (optionally, not on by default).
https://twitter.com/PrzemyslawKlys/status/1144342766408937490

Although Invoke-ScriptAnalyzer has a -Fix switch, the switch is only on for the -Path parameter set, therefore being able to optionally run this rule in the formatter is going to make it easier in the future to add a setting to vs code to optionally correct aliases. This change does not change the defaults behavior of Invoke-Formatter, it enables the following scenario to work

> Invoke-Formatter -ScriptDefinition gci -Settings @{IncludeRules=@('PSAvoidUsingCmdletAliases'); Rules=@{'PSAvoidUsingCmdletAliases'=@{}}}
Get-ChildItem

Yes, the current debt of the code base is not ideal that invoke-formatter has the allowed rules hard-coded but given the narrow use case of it, refactoring it would be a separate item. I cannot even improve it to use resource strings because they are in the Rules project and the Rules project has a dependency on the Engine project (or would you want me to move the resource strings into the Engine project?)

cc @PrzemyslawKlys

PR Checklist

@bergmeister bergmeister marked this pull request as ready for review June 30, 2019 12:50
@JamesWTruher
Copy link
Member

@bergmeister my real concern is the inclusion of this sole rule as an option. Shouldn't we allow any/all the rules to be run?
FWIW - It seems more like the Formatter.Format API is a special instance of the Fix which is perhaps misguided. In the end, It's all corrective measures, some of which are specifically targeting the formatting of the script but we don't have a way to distinguish a formatting change from an actual correction, which is something that RuleInfo should probably have.

@bergmeister
Copy link
Collaborator Author

Yes, I agree but addressing this technical debt of PSSA's architectural design is non trivial and is a separate issue to me. With this proposed change I'm not adding to the debt.

@travisclagrone
Copy link
Contributor

Yes, I agree but addressing this technical debt of PSSA's architectural design is non trivial and is a separate issue to me. With this proposed change I'm not adding to the debt.

@bergmeister Would you be able to put in one or more issues to fix this particular case of PSScriptAnalyzer's architectural debt? I'd like to take a crack at fixing it myself (or at least jump-start a discussion), considering that I'm looking at extending the PSScriptAnalyzer<=>PowerShellEditorServices interface regarding completions and intellisense sometime over the next few months.

@bergmeister
Copy link
Collaborator Author

bergmeister commented Jul 6, 2019

@travis-c-lagrone The debt is that the formatter has a list of hard coded rules that it can execute only. As said before, it should be improved so that whilst we might still want to have a hard-coded list of 'default' rules, it would be nice that if one specified any rule in the settings file/object that Invoke-formatter executes them. It would need some research as to why this is the case, I don't know more than that.
With respect to PSSA-PSES integration, the next item on the roadmap is to produce a NuGet package of PSSA so that PSES calls the .NET APIs directly for improved performance (and it also means that PSES can receive updates without requiring PSSA to do a full release.
Some of the other technical debt is the VariableAnalysis classes that were forked from PowerShell's own classes 4 years ago. Those classes were not updated since and have small bugs that leads to certain types of analysis not being possible. Unfortunately, the original maintainers have taken only a subset of it and added a lot of custom code to it. I once tried to just delete it and replace it with the code from PowerShell and some of the challenges were that SMA are very entangled with each other in terms of dependencies and it compiles against .Net Core, but PSSA compiles against .Net Standard...

@travisclagrone
Copy link
Contributor

... I once tried to just delete it and replace it with the code from PowerShell and some of the challenges were that SMA are very entangled with each other in terms of dependencies and it compiles against .Net Core, but PSSA compiles against .Net Standard...

Two questions:

  1. By "SMA", do you mean the System.Management.Automation .NET namespace?
  2. Is there a particular reason PSSA still supports PowerShell 3? (I ask because it looks like a fair bit of the custom code is because of that) Should PSSA still support PS3 considering that 5.1 is the officially recommended version of WinPS now?

@bergmeister
Copy link
Collaborator Author

bergmeister commented Jul 7, 2019

Yes, SMA is System.Management.Automation, which the core part of the PowerShell engine.
We plan to make a major version bump to PSSA 2.0 where support for PS v3/4 gets dropped. Current 'support' is just based on which PS versions are supported by MSFT but dropping support for v3/4 only simplifies some custom code but does not solve the problem with the forked VariableAnalysis code from SMA. We will probably have to support v5.1 for a long time so we will never get around the thing that we do conditional compilation.

@travisclagrone
Copy link
Contributor

When does the PSSA team plan to make the bump to PSSA 2.0? And is there a list of changes somewhere that will/can/should be made concurrently to take advantage of that bump?

@JamesWTruher JamesWTruher merged commit abd88a8 into PowerShell:master Aug 27, 2019
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

3 participants