search

dprint / dprint-intellij

A dprint plugin for Intellij.
https://plugins.jetbrains.com/plugin/18192-dprint
MIT License
12 stars 2 forks source link

Cache whether a file is formattable #19

Closed ryan-rushton closed 2 years ago

ryan-rushton commented 2 years ago

Since we are using the editor service to check whether a file is formattable in the new external formatting service we should cache the output so we only need to hit dprint once. That method is not supposed to be blocking and the fact we are communicating with the editor service is not something we should do but we do need to do it at least once.

In this we will also need to add an action so people can clear the cache if they are changing their config.

dsherret commented 2 years ago

@ryan-rushton with the upcoming new editor service we will have async messages and sending the message to ask if a file should be formatted should be very fast. I’d recommend waiting for that and seeing how it performs before seeing whether to implement this. I can start work on the new editor service this weekend.

ryan-rushton commented 2 years ago

Awesome and happy too.

I do actually wonder if the communication layer itself is too slow for this task though, i.e. reading and writing to the process and the fact that I am now synchronizing calls in that draft PR I have up. If a bulk format is triggered we might have a lot of blocking tasks pile up and the only way I see that not being an issue is making dprint handle multiple formatting requests at the one time, which is probably a bit of an ask. I think it's worth waiting and seeing how we go though.

One other thing I have noticed is that IntelliJ is super chatty to this method, it seems to get called twice for every format with these changes that I have put up.

dsherret commented 2 years ago

Is the canFormat(file: PsiFile): Boolean method not async? That's kind of annoying. I think it would need to block in that case, which might mean caching this might make sense.

If a bulk format is triggered we might have a lot of blocking tasks pile up and the only way I see that not being an issue is making dprint handle multiple formatting requests at the one time, which is probably a bit of an ask.

This is implemented in https://github.com/dprint/dprint/pull/505

I think we would need to extract out the process communication from the DprintService.kt file then have a common interface that would be implemented for schema version 4 and 5. Eventually then version 4 can be deprecated. I can look into writing some of the code maybe next week for this plugin... not sure I'll be able to do it all though because I've never programmed in Kotlin.

ryan-rushton commented 2 years ago

Is the canFormat(file: PsiFile): Boolean method not async?

No it isn't. It wasn't needed initially but now in the external formatter it can't. When it is used here, we are using it in a blocking method. For example in the Rust formatter they just return whether the file has a .rs extension. Unfortunately it isn't that easy for us.

I think we would need to extract out the process communication from the DprintService.kt file then have a common interface that would be implemented for schema version 4 and 5.

Yeah this was my plan too. I think we could either extract out an interface or extract out an abstract class where the abstract class implements all the stuff to start and stop the editor service, then the extensions of it implement the communication protocols.

I am happy to pick up this work but also happy for you to do it if you want to get your hands dirty with some kotlin :)

dsherret commented 2 years ago

Yeah this was my plan too. I think we could either extract out an interface or extract out an abstract class where the abstract class implements all the stuff to start and stop the editor service, then the extensions of it implement the communication protocols.

Cool, that sounds good!

I am happy to pick up this work but also happy for you to do it if you want to get your hands dirty with some kotlin :)

If you have the bandwidth for it that would be great... I think doing this would take me a long time 😅. This is what I did for the vscode plugin:

https://github.com/dprint/dprint-vscode/pull/30/files#diff-25a12db53dbfb719567f9feeb9444f8ceb525f8588a9f4ad41a56e17ed4b88ac

I haven't tested it thoroughly, but it seems to work.

ryan-rushton commented 2 years ago

Yeah I can do that and I don't mind, I will do it over the next couple of weeks.

Using the message id to send and receive fragments looks good. In the kotlin plugin there is only ever once instance of the DprintService due to how their service implementations work, so adapting this should be relatively straight forward.

ryan-rushton commented 2 years ago

Closing this as it has actually been implemented now.