Skip to content

Add New-EditorFile #622

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

Merged
merged 4 commits into from
Feb 20, 2018
Merged

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Feb 6, 2018

Here's what you can do:

Open-EditorFile foo.txt -Force

This will create a new file called foo.txt if no file exists. It will throw an error if you don't add -force and foo.txt does not exist.

Also,

Get-Process | Open-EditorFile foo.txt -Force

This will create a new file called foo.txt with the contents of what came in through the pipeline. Another example:

irm https://raw.githubusercontent.com/PowerShell/PowerShell/master/tools/install-powershell.ps1 | psedit install-powershell.ps1 -Force

You can also do:

Get-Process | Open-EditorFile

This will open an Untitled file in your editor and put the contents in it.

This works in local and remote sessions.

Note, Get-Process has an issue in remote sessions when piping to a file. I originally thought it was my code but it was not:
PowerShell/PowerShell#6115

The vscode-powershell PR is here: PowerShell/vscode-powershell#1197

Resolves #414
Resolves #413

$FilePaths,

[Parameter(ValueFromPipeline=$true)]
$NewValue,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set-Content uses Value as the name for this parameter, maybe good to be consistent since the behavior is similar? Not really a big deal, though.

end {
$FilePaths | ForEach-Object {
if (-not (Test-Path $_) -and $Force) {
$container > $_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely would be interested in Keith's feedback about this vs Set-Content -Force

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in the context of file manipulation the Force parameter is usually associated with dealing with read only files.

I'm also a bit confused about the purpose of this command. When I see the name Open-EditorFile I don't associate that with setting an initial content (NewValue). Isn't there a New-EditorFile which would take an optional Value (as well as Force to overwrite existing files)?

I think this command is trying to do a bit too much. Seems like it should just open the specified file in the editor workspace. Folks can then use the $pseditor api to modify its contents.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the function does change value, I think it makes more sense to change it via the $psEditor API (with $psEditor.GetEditorContext().CurrentFile.InsertText()) instead of directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkeithhill New-EditorFile does not exist which is why I bundled it with Open-EditorFile but I guess we can just have another function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be cleaner and more PowerShelly. :-)

param(
[Parameter(Mandatory=$true)]
[ValidateNotNullOrEmpty()]
$FilePaths,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be singular and in this case I think Path is sufficient and consistent with the other PowerShell file manipulation commands.

Also, I think it would be better to have this command accept pipeline input for the path instead of content. But that requires some changes to the parameter config - see the Examples\PathProcessingWildcards.ps1 for an example of how to do that.

With this change, I would be able to do:

gci *.psd1 -r | Open-EditorFile

That seems more useful to me than using the pipeline to overwrite (or create) files with new content.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do both:

function Open-EditorFile {
    [CmdletBinding(PositionalBinding=$false, DefaultParameterSetName='Path')]
    param(
        [Parameter(Mandatory, ParameterSetName='Path', ValueFromPipeline, ValueFromPipelineByPropertyName, Position=0)]
        [Parameter(Mandatory, ParameterSetName='Value', Position=0)]
        [ValidateNotNullOrEmpty()]
        [Alias('FullName')]
        [string] $Path,

        [Parameter(ParameterSetName='Value', ValueFromPipeline)]
        [Parameter(ParameterSetName='Path')]
        [ValidateNotNullOrEmpty()]
        [string] $Value
    )
    begin {
		if ($MyInvocation.ExpectingInput) {
            $valueList = [System.Collections.Generic.List[string]]::new()
        }
    }
    process {
        if ($PSCmdlet.ParameterSetName -eq 'Value') {
            $valueList.Add($Value)
            return
        }

        $psEditor.Workspace.OpenFile($Path)
        if ($Value) {
            $psEditor.GetEditorContext().CurrentFile.InsertText($Value)
        }
    }
    end {
        if ($PSCmdlet.ParameterSetName -eq 'Path') {
            return
        }

        $psEditor.Workspace.OpenFile($Path)
        $psEditor.GetEditorContext().CurrentFile.InsertText($valueList -join [Environment]::NewLine)
    }
}

Allows for gci *.ps1 | Open-EditorFile -Value $x and gc .\sourceFile.ps1 | Open-EditorFile destinationFile.ps1.

@TylerLeonhardt
Copy link
Member Author

I tried SO hard to get C# to read in a psm1 as a string... but eventually gave up and put the whole module in a string instead...

The benefit of this approach is that we can add more functions to the module string and they will be automatically added to remote sessions.

@TylerLeonhardt TylerLeonhardt changed the title Add new file capabilities to psedit Add New-EditorFile Feb 8, 2018
} else {
$PSCmdlet.WriteError( (
New-Object -TypeName System.Management.Automation.ErrorRecord -ArgumentList @(
[System.Exception]'File already exists.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we model this error record after New-Item?

New-Object -TypeName System.Management.Automation.ErrorRecord -ArgumentList @(
    [System.IO.IOException]"The file '$fileName' already exists.",
    'NewEditorFileIOError',
    [System.Management.Automation.ErrorCategory]::WriteError,
    $fileName)

foreach ($fileName in $Path)
{
if (-not (Test-Path $fileName) -or $Force) {
$container > $fileName
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see this edit the file through $psEditor instead of directly.

$psedit = New-Module -ScriptBlock ([Scriptblock]::Create($PSEditModule)) -Name PSEdit
$psedit.ExportedFunctions.Keys | ForEach-Object {
Set-Item -Path function:\global:$_ -Value $asdf.ExportedFunctions[$_].Definition -Force
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going the module route, can we pipe this into Import-Module instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SeeminglyScience I tried Import-Module initially but it did not work. None of the cmdlets would show up... Could I have been missing a particular flag? I tried -Force and -Scope.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerl0706 try -Global. I thought you only needed that when importing from a different session state, but I guess it's scope based. TIL

{
Remove-Item -Path 'function:\global:Open-EditorFile' -Force
Get-Command | Where-Object {$_.Source -eq 'PSEdit'} | ForEach-Object {
Remove-Item -Path function:\global:$($_.Name) -Force
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, Get-Module PSEdit | Remove-Module would be preferable

@TylerLeonhardt
Copy link
Member Author

Ok I've addressed all of @SeeminglyScience's feedback.

I've also added a small additional feature to this... You can now do:

Get-Process | Open-EditorFile

This will open an Untitled file in your editor and put the contents in it.

This works in local and remote sessions.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fantastic, great job man! :)
Assuming the AppVeyor build gets it's act together of course -_-

@TylerLeonhardt
Copy link
Member Author

@daviwil this PR is substansially different than when you signed off. Can you look again?

Also, @rkeithhill, I've addressed your feedback.

param(
[Parameter(Mandatory=$true, ValueFromPipeline=$true)]
[ValidateNotNullOrEmpty()]
$Path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as this parameter supports this scenario Get-ChildItem *.ps1 -r -file | Open-EditorFile, I'm good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So @rkeithhill ... as it turns out, this will proceed to open the file temporarily (with the italicized file name)... so what ends up happening is, it opens the file temporarily and then the next file opens and replaces the file temporarily.

I'm not sure how to durably open files - if that's possible. I'll have to dig around.

@daviwil any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. That's why I double-click the editor window tab. That converts it from a temp (resuable) window to a permanent one. Not sure how you do that programmatically though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. Maybe if @SeeminglyScience or @daviwil know? Otherwise, can I convince you to call this out of scope 😄

Copy link
Contributor

@rkeithhill rkeithhill Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the vscode.window.showTextDocument() overload that takes a TextDocumentShowOptions object. One of the fields of that object is preview?. That should allow you to create each window in a "non-preview" window.

https://code.visualstudio.com/docs/extensionAPI/vscode-api#_a-nametextdocumentshowoptionsaspan-classcodeitem-id239textdocumentshowoptionsspan

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be my preference. Later we could update the openFile() method to take an optional "preview" Boolean parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later we could update the openFile() method to take an optional "preview" Boolean parameter.

What if I already have this working? Is it worth adding it now or saving it for when someone actually asks for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just push and you can judge whether we should keep it in or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have that option, so yeah, if it's working - go for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkeithhill pushed that. Here's the vscode-powershell PR as well:
PowerShell/vscode-powershell#1197

@TylerLeonhardt
Copy link
Member Author

Ah! This resolves this as well:
#413

@TylerLeonhardt
Copy link
Member Author

oh... appveyor.

@TylerLeonhardt
Copy link
Member Author

Can you guys give a thumbs up to this if you still approve? There have been some substantial (enough) changes that those approvals don't really stand anymore.

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, merge it

@TylerLeonhardt TylerLeonhardt merged commit f534ae6 into PowerShell:master Feb 20, 2018
@TylerLeonhardt TylerLeonhardt deleted the psedit-new-files branch February 20, 2018 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants