-
Notifications
You must be signed in to change notification settings - Fork 227
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
Move PSReadLine logic to cmdlets #1255
Move PSReadLine logic to cmdlets #1255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Couple requests
/// The Start-EditorServices command, the conventional entrypoint for PowerShell Editor Services. | ||
/// </summary> | ||
[Cmdlet("__Invoke", "ReadLineForEditorServices")] | ||
public sealed class InvokeReadLineForEditorServicesCommand : PSCmdlet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop the Cmdlet
decoration and make this internal. When you go to invoke it, use:
ps.AddCommand(
new CmdletInfo(
"any-hyphenseparatedname",
typeof(InvokeReadLineForEditorServicesCommand)))
(and probably staticly store the CmdletInfo
)
Edit: hmm I guess the direct CommandInfo
route isn't accessible from PSCommand
directly for some reason. Either PSCommand.AddCommand
needs an overload for CommandInfo
, or the Runspaces.Command
ctor that takes it needs to be made public.
I'd honestly still recommend going this route, and use reflection to access the Command.ctor(CommandInfo)
overload. Then add an internal extension method PSCommand.AddCommand(CommandInfo)
so we can use this as necessary.
I really dislike adding global state, though I understand if other folks aren't against it enough to think this approach is warranted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to pass on this one for now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think I explained that well. That part in particular should be pretty easy, this is what I mean:
internal static PSCommand AddCommand(this PSCommand command, CommandInfo commandInfo)
{
var rsCommand = (Command)typeof(Command)
.GetConstructor(
BindingFlags.Instance | BindingFlags.NonPublic,
binder: null,
new[] { typeof(CommandInfo) },
modifiers: null)
.Invoke(new object[] { commandInfo });
return command.AddCommand(rsCommand);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually can't do this without breaking the contract between the PSES.Host proj and PSES proj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can put the command in PSES...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually then that loads it in a different ALC... ok there's a bit too much going on here. I'm gonna defer this.
src/PowerShellEditorServices.Hosting/Commands/InvokeReadLineForEditorServicesCommand.cs
Outdated
Show resolved
Hide resolved
Issues
======
- Added 1
Complexity increasing per file
==============================
- src/PowerShellEditorServices.Hosting/Commands/InvokeReadLineConstructorCommand.cs 1
- src/PowerShellEditorServices.Hosting/Commands/InvokeReadLineForEditorServicesCommand.cs 1
See the complete overview on Codacy |
|
||
// TODO: Handle method being null here. This shouldn't ever happen. | ||
|
||
return (ReadLineInvoker)method.CreateDelegate(typeof(ReadLineInvoker)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue found: 'method' is null on at least one execution path.
fixes PowerShell/vscode-powershell#2623
This should enable the ability to start up with ConstrainedLanguage mode (though I didn't remove the check because I wasn't sure if I could set it on non-Windows). There are features that won't work though - expand alias, remoting, Command explorer, etc.