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

Black edits being deferred by Nova during save operations #1

Closed
Anton-2 opened this issue Sep 27, 2020 · 4 comments · Fixed by #4
Closed

Black edits being deferred by Nova during save operations #1

Anton-2 opened this issue Sep 27, 2020 · 4 comments · Fixed by #4

Comments

@Anton-2
Copy link

Anton-2 commented Sep 27, 2020

Blake process is asynchronous, and acts on the editor's buffer, but nova will save the file before Blake's actions.
You see the file after Blake's edit, but the file saved is the previous version.
This led to strange flake reports on formatting issues, as flake8 will read the saved file, but report it on the version corrected by Blake, which is correct.

I didn't find any way to tell nova to wait for the end of edit before saving, so I don't know if it can work correctly with the current nova API.

@Anton-2 Anton-2 changed the title Bake on save is modifiing When using blake on save, the file is saved before blake's modifications Sep 27, 2020
@flother
Copy link
Owner

flother commented Sep 28, 2020

Thanks for creating an issue for this. It's always a pleasure to hear that people are using things I make.

Blake shells out to a Black process within an editor.onWillSave() callback. The Nova docs say that:

If the callback performs modifications to the editor within the callback (such as with edit()), they will be applied in such a way as to include them in the pending save operation. If a callback registered using this method takes too long to perform operations, any edits enqueued may be deferred until after the save operation, or discarded entirely. (source)

Blake does alter the document using an edit() callback, as the docs suggest, and so Blake's edits should be included in save operations. But, as you've noticed, they aren't: as well as the mismatch with Flake8 issues, the document is also marked as unsaved after Black's changes have been applied.

There's no mention of how long is "too long", but I think the process of running Black within a shell is taking too long to return, and so the edits are being deferred. I'll have to rethink how I'm running Flake8 and Black on save, perhaps try and run them synchronously one after the other, using Black's output as input to Flake8.

@flother flother changed the title When using blake on save, the file is saved before blake's modifications Black edits being deferred by Nova during save operations Sep 28, 2020
@Anton-2
Copy link
Author

Anton-2 commented Sep 28, 2020

And thanks for creating this...

I've tried to make the black round-trip faster, by using blackd and just POST'ing a request, but this is still not fast enough.
I suspect that anything doing IO will take "too long"...

To implement a true "format before save" option, we need to have a way in nova's API to defer save until all edits are done. I've asked Panic support about this.

@edwardloveall
Copy link
Contributor

I don't think it's the time it takes to run black. From the Nova docs for onWillSave:

the runtime guarantees that it will allow at least 5 seconds for all extensions to perform their operations.

Black is definitely not taking that long! 😅

The problem is that onWillSave needs to return a Promise if you want it to wait:

This method may return a Promise object to denote to the runtime that changes need to be performed asynchronously.

Right now, the onWillSave callback doesn't return anything. The BlackProcess does its work in the background and when it completes without any errors, it alters the document. But by the time this has happened, onWillSave is already done since it didn't know to wait.

I've been understanding this through an extension I'm working on and I'd be happy to take a crack at making it work here. However, playing around with the code a little, I believe it will be a significant refactor of at least BlackProcess and likely to Formatter as well so I didn't want to give it a go without checking first.

@edwardloveall
Copy link
Contributor

I ended up making these changes in #4. Even if they don't get merged, we can at least see what it might look like.

@flother flother closed this as completed in #4 Jan 5, 2023
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 a pull request may close this issue.

3 participants