Add `PluginId` to `CommandFunction` & make `getNormalizedFilePath` takes `Uri` as pram instead of `TextDocumentIdentifier` #2906

July541 · 2022-05-14T10:35:03Z

Further on this comment, I found a lot of plugins can benefit form this change.

I keep the pattern uriToNormalizedFilePath . toNormalizedUri if their result is wrapped by MaybeT.

Some changes are from pre-commit hook.

eddiemundo

Not sure if changing CommandFunc signature is necessary since some of them don't use the PluginId and it seems like you can always get a PluginId from the descriptor or nearby if you do need it. The changes in the hlint plugin to reduce duplication and have slightly better error messages are good, but can also be done without changing CommandFunc.

July541 · 2022-05-14T16:26:11Z

Not sure if changing CommandFunc signature is necessary since some of them don't use the PluginId and it seems like you can always get a PluginId from the descriptor or nearby if you do need it. The changes in the hlint plugin to reduce duplication and have slightly better error messages are good, but can also be done without changing CommandFunc.

If your always get a PluginId from the descriptor or nearby if you do need it means write function like foo :: PluginId -> CommandFunction .., it is. But we can also get IdeState nearby, I think the CommandFunc is the synonym of what we need to build command in most cases, it's the only identifier to identify a plugin, and furthermore when we have more functions into hls-plugin-api, we can call these functions easily without explicitly passing PluginId.

drsooch · 2022-05-14T20:26:54Z

I'm good with the change, I actually planned on tackling this at some point but haven't got around to it.

eddiemundo · 2022-05-14T21:00:57Z

I see your point about IdeState. I'm not opposed to it, just throwing out some thoughts.

michaelpj · 2022-05-16T09:22:54Z

hls-plugin-api/src/Ide/Types.hs

@@ -389,6 +389,7 @@ data PluginCommand ideState = forall a. (FromJSON a) =>

 type CommandFunction ideState a
  = ideState
+  -> PluginId


I'm not sure this is an improvement. The other arguments to CommandFunction are things that are only known when the command is run, so we need to pass them in then. But the PluginId is known when the plugin is created, so we can easily pass it in there. Doing it that way is pretty straightforward, as the tactics plugin does today.

Plus, most of the existing ones don't use it, so we've also added a bunch of unused parameters...

Only the command parameter is needed to be passed, I think ideState has the same condition as pluginId, we can also pass ideState while we only need it.

Here are the stat about the usage of CommandFunction.

8/11 use ideState, 3/11 use PluginId, and most of plugins don't use PluginId because they use MaybeT to wrap NormalizedFilePath conversion, I keep this pattern in the pr. I will respect your view if you still believe this change is of little significance.

plugin name ideState PluginId

completion ✓ ×

typelense × ×

class ✓ ×

eval ✓ ✓

explicit-import × ×

hlint ✓ ✓

module-name ✓ ×

refine-import × ×

retrie ✓ ×

splice ✓ ×

tactics ✓ ✓

If I may add, like I mentioned earlier I want to clean up all of the duplicate code revolving around MaybeT. So I would plan on removing the MaybeT wrappers. It may end up being more daunting but I expect to need to use some sort of identifier to use in logging messages.

This would also extend to other commonly requested items as well. So it would be nice to have some generic identifier to pass around.

TBH I also think it looks like we shouldn't pass IdeState like that, if it truly is globally constant. I'm a bit suspicious that it isn't truly constant though, we should check.

I think my argument is that it's unclear what should be part of CommandFunc. Just any convenient stuff? Then there's really no rule about what to put in there or not. Whereas with my proposal there is a simple rule: it should take exactly the stuff that's only available when you run the command, and nothing else. If you need anything else, just pass it in when you construct the plugin.

I'm a bit suspicious that it isn't truly constant though, we should check.

I think my changes have covered all usage of CommanFunc.

I think my argument is that it's unclear what should be part of CommandFunc. Just any convenient stuff? Then there's really no rule about what to put in there or not.

I believe CommandFunc is an abbreviation of its arguments and its result, and the arguments can be anything you may use.
I prefer to add something that may not be used to keep the CommandFunc be unified in most situations.

So I would plan on removing the MaybeT wrappers.

Agree with this.

As I said earlier, I'm going to close this and open a new pr to make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier only.

michaelpj · 2022-05-16T09:23:04Z

hls-plugin-api/src/Ide/PluginUtils.hs

@@ -246,13 +246,12 @@ allLspCmdIds pid commands = concatMap go commands

 -- ---------------------------------------------------------------------

-getNormalizedFilePath :: Monad m => PluginId -> TextDocumentIdentifier -> ExceptT String m NormalizedFilePath
-getNormalizedFilePath (PluginId plId) docId = handleMaybe errMsg
+getNormalizedFilePath :: Monad m => PluginId -> Uri -> ExceptT String m NormalizedFilePath


It is indeed much clear and easy to use, I saw many codes can use uri directly instead of TextDocumentIdentifier.🎉

July541 requested review from isovector, konn, pepeiborra, jneira, eddiemundo, Ailrun, drsooch and berberman as code owners May 14, 2022 10:35

July541 changed the title ~~Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier`~~ Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier May 14, 2022

Add PluginId to CommandFunction

0c187d4

July541 force-pushed the command-plId branch from c8428f9 to 0c187d4 Compare May 14, 2022 10:43

July541 mentioned this pull request May 14, 2022

A plugin for GADT syntax converter #2899

Merged

July541 added 2 commits May 14, 2022 18:57

Add missing imports

396642c

Remove redundant imports

5f27422

eddiemundo reviewed May 14, 2022

View reviewed changes

michaelpj reviewed May 16, 2022

View reviewed changes

July541 closed this May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `PluginId` to `CommandFunction` & make `getNormalizedFilePath` takes `Uri` as pram instead of `TextDocumentIdentifier` #2906

Add `PluginId` to `CommandFunction` & make `getNormalizedFilePath` takes `Uri` as pram instead of `TextDocumentIdentifier` #2906

July541 commented May 14, 2022 •

edited by gitpod-io bot

Loading

eddiemundo left a comment

July541 commented May 14, 2022

drsooch commented May 14, 2022

eddiemundo commented May 14, 2022

michaelpj May 16, 2022

michaelpj May 16, 2022

July541 May 16, 2022

drsooch May 16, 2022

michaelpj May 16, 2022

July541 May 17, 2022

michaelpj May 16, 2022

July541 May 16, 2022

plugin name	ideState	PluginId
completion	✓	×
typelense	×	×
class	✓	×
eval	✓	✓
explicit-import	×	×
hlint	✓	✓
module-name	✓	×
refine-import	×	×
retrie	✓	×
splice	✓	×
tactics	✓	✓

Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier #2906

Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier #2906

Conversation

July541 commented May 14, 2022 • edited by gitpod-io bot Loading

eddiemundo left a comment

Choose a reason for hiding this comment

July541 commented May 14, 2022

drsooch commented May 14, 2022

eddiemundo commented May 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `PluginId` to `CommandFunction` & make `getNormalizedFilePath` takes `Uri` as pram instead of `TextDocumentIdentifier` #2906

Add `PluginId` to `CommandFunction` & make `getNormalizedFilePath` takes `Uri` as pram instead of `TextDocumentIdentifier` #2906

July541 commented May 14, 2022 •

edited by gitpod-io bot

Loading