-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix bug #613 ":wq command dows not work" #630
Conversation
needed to fs.accessSync to return the saving promise
It was too late yesterday didn't get it how |
No save as dialog is required because Vim doesn't use one. :) Do you think this is good to go, or do you need any help? |
I would like to implement the tests for |
Nothing looks horrifically stupid to me. :) Anything in particular you want us to look at? |
No nothing special, just a bit insecure about the async/await promise stuff ;). I just cleaned up the test code a bit (cut out the ugly polling). I think it's good to go. |
:) you aren't the only one, we were having a discussion in slack about exactly that earlier today! |
@@ -37,7 +37,7 @@ export class WriteCommand extends node.CommandBase { | |||
return this._arguments; | |||
} | |||
|
|||
execute(modeHandler : ModeHandler) : void { | |||
async execute(modeHandler : ModeHandler) : Promise<void> { |
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, this one doesn't need to be async because you don't do any awaiting inside it.
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 need it to return the encapsulating promise from the activeTextEditor.document.save()
call in the save method. otherwise i wasn't able to call write
cmd and quit
cmd synchronously.
modeHandler.setupStatusBarItem(accessErr.message); | ||
} | ||
try { | ||
fs.accessSync(this.activeTextEditor.document.fileName, fs.W_OK); |
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.
Hey can you help me understand why we use sync
instead of async
methods here?
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.
Hi @rebornix, with my restricted type/javascript skills i couldn't return the Save
method Promise, and if i'm not able to wait until the editor has saved the file i can not close it with the close-cmd.
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 not super concerned about blocking during a save... should I be?
Let me know if this one is good to go, @Platzer! |
@johnfn I would say so, but i've got the developer sickness > never believe my code is done (especially on new terrain)! |
Looks fine to me - thanks, @Platzer! |
Bug fix is currently not working with Visual Studio Code - Insiders
it seems that the
isDirty
property of thevscode.TextDocument
is set false for newly created document. Going to check it tomorrow with normal vscode.