-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Description
Plan:
- - Get feedback for removing ScopeItemOption.AllScope from alias definitions in InitialSessionState.cs.
- - Add internal only support for ScopeItemOption settings in AliasAttribute.
- - Migrate alias declarations from InitialSessionState.cs to files with cmdlet implementations.
- - Add tests to control using
ScopeItemOptionin alias definitions. (Done in Change to not expose unsupported aliases/cmdlets in Unix #3595) - - Add tests to control exporting aliases. (Done in Change to not expose unsupported aliases/cmdlets in Unix #3595)
Motivation is from #3595 (#3595 (comment))
My opinion doesn't stem from the performance concern. I just think we should no longer keep stuffing unnecessary settings to InitialSessionState.cs . Keep alias declaration in individual cmdlet is good for at least 2 reasons:
1.alias declaration is together with the cmdlet definition -- easy to read/discover and the alias exists in the cmdlet as a metadata information which can be analyzed using reflection by tools (if there is any tool doing so).
2.we want to stop the pattern of declaring aliases in a centralized location, so that when multiple people working on different cmdlets at the same time, they will have less conflicts that are resulted from editing the same file.
For those existing alias declarations in InitialSessionState.cs , we cannot move them out due to the ScopeItemOption settings (BTW, I think for some of the aliases there, the authors probably just copied the existing options without knowing if they should actually be applied). But for new aliases, I think it's better to have them coupled with the cmdlet unless an option other than 'none' is really desired.
There are other benefits to the Alias attribute:
- AllScope is terrible for performance - there is little benefit to have most of these aliases as AllScope
- Consistency - if a non-default runspace is constructed and imports one of these modules, that runspace would not get the aliases defined by default. For example:
function DoIt($createArg)
{
[powershell]::Create($createArg).
AddScript('$ExecutionContext.InvokeCommand.GetCommand("clc", "Alias")').
Invoke()
}
DoIt ([System.Management.Automation.RunspaceMode]::NewRunspace)
DoIt ([System.Management.Automation.RunspaceMode]::CurrentRunspace)
$iss = [InitialSessionState]::Create()
$iss.ImportPSModule('Microsoft.PowerShell.Management')
$iss.LanguageMode = 'FullLanguage'
DoIt $issThis prints:
CommandType Name Version Source
----------- ---- ------- ------
Alias clc -> Clear-Content
Alias clc -> Clear-Content
Note - only 2 results instead of 3. This is a little weird because Clear-Content is in
Microsoft.PowerShell.Management, so why isn'tclc?
We should perhaps consider adding support for AllScope/ReadOnly in the Alias attribute, perhaps as an internal only option so that it isn't abused.