Change Hsimport to use configured Formatter on import #1167

fendor · 2019-04-08T21:16:36Z

Closes #1115

Adds a function to get the configured formatter plugin.
Configures HsImport to use the configured formatter.

Add PluginCommands to format lines of text for Birttany and Floskell.
Add FormatTextCmdParams for the Cmd.

TODO:

~~[ ] Formatting Options should be honored.~~ Will not be adressed in this PR.

src/Haskell/Ide/Engine/Plugin/HsImport.hs

lukel97 · 2019-04-08T21:41:36Z

src/Haskell/Ide/Engine/Support/HieExtras.hs

+-- ---------------------------------------------------------------------
+
+getFormattingPlugin :: Config -> IdePlugins -> Maybe (PluginDescriptor, FormattingProvider)
+getFormattingPlugin config plugins = do 


Can this happen inside R so Config and IdePlugins don’t need to be passed?

Hardly, since IdeGhcM does not implement R, or can execute R, so other plugins would have no access to it.

I see, I didn't notice it was being called elsewhere outside of LspStdio.hs. I feel like this should be moved into PluginIdeMonads.hs though, and made polymorphic over MonadIde, which should have access to both. We'll need to figure out how to give R access to it though

fendor · 2019-04-08T23:20:38Z

src/Haskell/Ide/Engine/Support/HieExtras.hs

+getFormattingPlugin :: Config -> IdePlugins -> Maybe (PluginDescriptor, FormattingProvider)
+getFormattingPlugin config plugins = do 
+  let providerName = formattingProvider config
+  fmtPlugin <- Map.lookup providerName (ipMap plugins)


This is actually wrong, since we look up the name, but the Plugins Map is of Type Map PluginId PluginDescriptor

But it works, since due to some lucky coincidence pluginId == pluginName.

I think it would be better if PluginId would be a newtype.

src/Haskell/Ide/Engine/Plugin/HsImport.hs

src/Haskell/Ide/Engine/Plugin/Brittany.hs

src/Haskell/Ide/Engine/Plugin/HsImport.hs

lukel97 · 2019-04-09T21:42:15Z

src/Haskell/Ide/Engine/Support/HieExtras.hs

+-- ---------------------------------------------------------------------
+
+getFormattingPlugin :: Config -> IdePlugins -> Maybe (PluginDescriptor, FormattingProvider)
+getFormattingPlugin config plugins = do 


I see, I didn't notice it was being called elsewhere outside of LspStdio.hs. I feel like this should be moved into PluginIdeMonads.hs though, and made polymorphic over MonadIde, which should have access to both. We'll need to figure out how to give R access to it though

lukel97 · 2019-04-11T20:54:59Z

@fendor I've created a PR for your PR on your fork, it changes HsImport to use the formatting providers directly, by applying TextEdits to TextEdits

fendor · 2019-04-12T11:58:25Z

Dont merge, yet, did a mistake while rebasing.

fendor · 2019-04-13T11:48:38Z

Ok, that was awkward. Although these are not clean commits, I think everything should be as expected now...

mpickering · 2019-04-13T16:15:09Z

src/Haskell/Ide/Engine/Plugin/Floskell.hs

+    let (range, selectedContents) = case typ of
+          FormatDocument -> (fullRange contents, contents)
+          FormatRange r  -> (r, extractRange r contents)
+        result = reformat config (uriToFilePath uri) (BS.fromStrict (T.encodeUtf8 selectedContents))


Why use uriToFilePath and not the file variable?

No particular reason, it was like that when I found it.

Includes changes from bubba.

mpickering

One small comment but otherwise I think looks fine. @bubba should give it his approval first.

mpickering · 2019-04-13T18:57:18Z

src/Haskell/Ide/Engine/Plugin/Brittany.hs

+  -> Uri
+  -> FormattingType
+  -> FormattingOptions
+  -> m (IdeResult [TextEdit])


Do you use this at another type of than IdeM?

No. It is an abstraction, so it might be used in another type, but not yet. Afaik, by using another Monad, it might be possible to either wait for a response or to get it immediately.
If this is bad style, we can drop it, though.

lukel97

Sorry it took me a while to get to this, this is great!

alanz · 2019-04-17T20:57:32Z

hie-plugin-api/Haskell/Ide/Engine/PluginsIdeMonads.hs

        -> FormattingType  -- ^ How much to format
        -> FormattingOptions -- ^ Options for the formatter
-        -> IdeDeferM (IdeResult [TextEdit]) -- ^ Result of the formatting or the unchanged text.
+        -> IdeM (IdeResult [TextEdit]) -- ^ Result of the formatting or the unchanged text.


This used to be IdeDeferM because brittany needs the GHC parsed source to work. Now that it is IdeM, will be potentially lose formatting under some circumstances?

Brittany's FormattingProvider doesn't use ifCachedModule/withCachedModule on master at the moment, so it looks like it wouldn't have been waiting for the parsed source

haskell-ide-engine/src/Haskell/Ide/Engine/Plugin/Brittany.hs

Lines 42 to 64 in a48a02c

provider :: FormattingProvider

provider uri formatType opts = pluginGetFile "brittanyCmd: " uri $ \file -> do

confFile <- liftIO $ getConfFile file

mtext <- readVFS uri

case mtext of

Nothing -> return $ IdeResultFail (IdeError InternalError "File was not open" Null)

Just text -> case formatType of

FormatRange r -> do

res <- liftIO $ runBrittany tabSize confFile $ extractRange r text

case res of

Left err -> return $ IdeResultFail (IdeError PluginError

(T.pack $ "brittanyCmd: " ++ unlines (map showErr err)) Null)

Right newText -> do

let textEdit = J.TextEdit (normalize r) newText

return $ IdeResultOk [textEdit]

FormatDocument -> do

res <- liftIO $ runBrittany tabSize confFile text

case res of

Left err -> return $ IdeResultFail (IdeError PluginError

(T.pack $ "brittanyCmd: " ++ unlines (map showErr err)) Null)

Right newText ->

return $ IdeResultOk [J.TextEdit (fullRange text) newText]

where tabSize = opts ^. J.tabSize

It looks like I moved it from IdeGhcM to IdeM in this commit, and in retrospect I should have given a second thought as to why it was In IdeGhcM in the first place.
runBrittany just runs in IO: does it access the same ghc-mod instance as HIE?

runBrittany just runs in IO: does it access the same ghc-mod instance as HIE?

Good question. I think once my HaRe update to use the new hie-bios is done we will have a clearer view of the API, and can ask brittany to expose something that makes use of the module caching

If it does not, then IdeDeferM makes no difference.

fendor · 2019-04-18T10:11:49Z

In my opinion, this is ready to merge.

fendor force-pushed the hsimport-use-formattingprovider branch 2 times, most recently from 647f45b to 90268c5 Compare April 8, 2019 21:26

fendor requested review from lukel97, alanz and wz1000 April 8, 2019 21:26

lukel97 reviewed Apr 8, 2019

View reviewed changes

fendor commented Apr 8, 2019

View reviewed changes

fendor commented Apr 9, 2019

View reviewed changes

src/Haskell/Ide/Engine/Plugin/HsImport.hs Outdated Show resolved Hide resolved

lukel97 reviewed Apr 9, 2019

View reviewed changes

fendor requested a review from lukel97 April 10, 2019 19:26

fendor mentioned this pull request Apr 10, 2019

HsImport importlist #1170

Merged

3 tasks

fendor force-pushed the hsimport-use-formattingprovider branch 2 times, most recently from d2cd15e to 3d77363 Compare April 11, 2019 10:11

fendor force-pushed the hsimport-use-formattingprovider branch from d0ac0d2 to f441449 Compare April 12, 2019 11:59

fendor added status: blocked and removed status: blocked labels Apr 13, 2019

mpickering reviewed Apr 13, 2019

View reviewed changes

Change HsImport to use the configured FormattingProvider

e5713e0

Includes changes from bubba.

fendor force-pushed the hsimport-use-formattingprovider branch from 3e97f32 to e5713e0 Compare April 13, 2019 18:07

Add more documenation and remove unused records

7ef6e4b

fendor force-pushed the hsimport-use-formattingprovider branch from 70baa51 to 7ef6e4b Compare April 13, 2019 18:37

mpickering reviewed Apr 13, 2019

View reviewed changes

lukel97 approved these changes Apr 17, 2019

View reviewed changes

alanz reviewed Apr 17, 2019

View reviewed changes

alanz merged commit 93718bd into haskell:master Apr 18, 2019

alanz added this to the 2019-04 milestone May 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Hsimport to use configured Formatter on import #1167

Change Hsimport to use configured Formatter on import #1167

fendor commented Apr 8, 2019 •

edited

Loading

lukel97 Apr 8, 2019

fendor Apr 8, 2019

lukel97 Apr 9, 2019

fendor Apr 8, 2019

fendor Apr 8, 2019

fendor Apr 8, 2019

lukel97 Apr 9, 2019

lukel97 commented Apr 11, 2019 •

edited

Loading

fendor commented Apr 12, 2019 •

edited

Loading

fendor commented Apr 13, 2019 •

edited

Loading

mpickering Apr 13, 2019

fendor Apr 13, 2019

mpickering left a comment

mpickering Apr 13, 2019

fendor Apr 13, 2019

lukel97 left a comment

alanz Apr 17, 2019

lukel97 Apr 17, 2019

alanz Apr 17, 2019

fendor commented Apr 18, 2019

	provider :: FormattingProvider
	provider uri formatType opts = pluginGetFile "brittanyCmd: " uri $ \file -> do
	confFile <- liftIO $ getConfFile file
	mtext <- readVFS uri
	case mtext of
	Nothing -> return $ IdeResultFail (IdeError InternalError "File was not open" Null)
	Just text -> case formatType of
	FormatRange r -> do
	res <- liftIO $ runBrittany tabSize confFile $ extractRange r text
	case res of
	Left err -> return $ IdeResultFail (IdeError PluginError
	(T.pack $ "brittanyCmd: " ++ unlines (map showErr err)) Null)
	Right newText -> do
	let textEdit = J.TextEdit (normalize r) newText
	return $ IdeResultOk [textEdit]
	FormatDocument -> do
	res <- liftIO $ runBrittany tabSize confFile text
	case res of
	Left err -> return $ IdeResultFail (IdeError PluginError
	(T.pack $ "brittanyCmd: " ++ unlines (map showErr err)) Null)
	Right newText ->
	return $ IdeResultOk [J.TextEdit (fullRange text) newText]
	where tabSize = opts ^. J.tabSize

Change Hsimport to use configured Formatter on import #1167

Change Hsimport to use configured Formatter on import #1167

Conversation

fendor commented Apr 8, 2019 • edited Loading

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

lukel97 commented Apr 11, 2019 • edited Loading

fendor commented Apr 12, 2019 • edited Loading

fendor commented Apr 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpickering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukel97 left a 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

fendor commented Apr 18, 2019

fendor commented Apr 8, 2019 •

edited

Loading

lukel97 commented Apr 11, 2019 •

edited

Loading

fendor commented Apr 12, 2019 •

edited

Loading

fendor commented Apr 13, 2019 •

edited

Loading