Use Brittany with hsimport when needed #783

lukel97 · 2018-08-25T01:42:20Z

This does a spartan check to see if the imports are formatted the way Brittany formats it, and if so then applies a coat of Brittany to the import code action.
Closes #772

Todo

Add LSP config option to disable this

lorenzo · 2018-08-25T10:20:30Z

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

+          return (any isFormattedLine ls)
+        -- Only use Brittany formatting if it's already formatted
+        isFormattedLine x
+          | "import qualified " `T.isPrefixOf` x = True


This is pretty much like most people write the qualified import, so it is a potential for a false positive. Maybe check the presence of at least two spaces after the next block of text?

Gurkenglas · 2018-08-25T10:26:20Z

The obvious way to check if it's formatted with britanny is to format it with brittany and see if that changes anything.

lorenzo · 2018-08-25T10:32:04Z

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

+              return $ IdeResultOk (J.WorkspaceEdit mChanges mDocChanges)
+
+  where hasFormattedImports fp = do
+          ls <- T.lines <$> liftIO (T.readFile fp)


Unrelated to this PR, can we consider not using the read function from the Text package? I’ve had problem in the past with locale settings messing up the results.

An alternative is to used ByteString and do a lenient decode to utf8

Could this same issue be causing #667?

Yes, that's exactly how the problem manifests. Should we create a ticket to get this addressed?

@lorenzo We can probably just reuse that ticket, and add some comments explaining that it could be related to Data.Text.IO.readFile. I just did a quick search and hie does use it in quite a few locations

I ran into similar issues when working on the PureScript IDE integration, if it's any help we're using these functions to read source files currently: https://github.com/purescript/purescript/blob/b07042fb7b4f6a68c0fdc68b7e51076b7106cfba/src/System/IO/UTF8.hs

…into brittany-hsimport

lukel97 · 2018-08-28T17:14:27Z

Updated to simply always apply Brittany formatting

alanz · 2018-10-28T10:18:20Z

I think this PR needs an update.

alanz · 2018-10-28T10:18:33Z

Then we can probably merge it.

…into brittany-hsimport

This allows for plugins to now access config options It also streamlines the process of running the monads from Scheduler Should be expanded upon to move VFS into

lukel97 · 2018-11-04T23:33:40Z

Not sure what I did differently but managed to add LspFuncs into IdeM, which now gives all plugins access to the Config so we can add an LSP option to disable this. This also turned out to have some great benefits and I was able to refactor Scheduler so that ideDispatcher and ghcDispatcher are symmetrical.
I plan on revising #861 so we can move VFS access straight into IdeM too!

lukel97 · 2018-11-05T00:52:17Z

Ready for re-review

lorenzo

I really like this refactoring. Left a few comments on things that could be improved

lorenzo · 2018-11-06T12:16:33Z

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

                 ]

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

 -- | For the diagnostic providers in the config, return a map of
 -- current enabled state, indexed by the plugin id.
-getDiagnosticProvidersConfig :: Config -> Map.Map PluginId Bool
+getDiagnosticProvidersConfig :: Config -> Map.Map T.Text Bool


Why is this different?

PluginId is defined inside PluginsIdeMonads, which now imports Config so its to avoid a circular dependency. This is probably a sign that we should split the module up a bit

Ah, makes sense

Should we not keep it as a PluginId, just define it in a different place? I am not all that happy with it sudennly being Text, we are losing coherence.

Can this function be moved into LspStdio.hs to avoid the circular dependency? It feels like it belongs in Config.hs though. I couldn't really find a logical grouping of Plugin related types to move out PluginsIdeMonads since they all reference IdeGhcM etc.
Another possibility is that PluginDescriptor gets moved into PluginsIdeMonads where the actual PluginDescriptor type lives

lorenzo · 2018-11-06T12:18:09Z

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

+    Just lf -> return (Core.rootPath lf)
+    Nothing -> return Nothing
+
+data IdeEnv = IdeEnv (Maybe (Core.LspFuncs Config))


Can you make this a newtype instead?

I meant to make this more than a wrapper for LspFuncs, for things that shouldn't change during the execution of IdeM/IdeGhcM/IdeDeferM. Updated it to include the PID and plugins

lorenzo · 2018-11-06T12:20:28Z

src/Haskell/Ide/Engine/Scheduler.hs

-  -> IO void
-ideDispatcher stateVar caps env errorHandler callbackHandler pin =
+  -> IdeM void
+ideDispatcher env errorHandler callbackHandler pin =
  -- TODO: AZ run a single ReaderT, with a composite R.


This TODO should be moved to the place where the code was moved to

Should be fixed

lukel97 · 2018-11-06T18:03:07Z

Looks like chocolately is down, AppVeyor build should be fine otherwise

Move getDiagnosticProvidersConfig there too

…into brittany-hsimport

lukel97 · 2018-11-10T01:50:00Z

@alanz @lorenzo is this ready to merge?

lorenzo · 2018-11-10T11:19:23Z

I’m ok with merging this

alanz · 2018-11-10T12:11:26Z

LGTM, merge when CI passes

alanz · 2018-11-10T12:12:49Z

BTW, I have seen intermittent failures with that test on various branches / ghc versions recently. I think we should try to make it more robust.

lukel97 added 3 commits August 25, 2018 02:38

Remove dump-logs options from tests

1c602ac

Format import code actions with Brittany if needed

953dd5d

Remove unused imports/functions

a04c940

lorenzo reviewed Aug 25, 2018

View reviewed changes

lukel97 added 3 commits August 28, 2018 18:03

Always format hsimport

e50e99a

Merge branch 'master' of https://github.com/haskell/haskell-ide-engine …

549d614

…into brittany-hsimport

Fix tests

dbf3eab

lorenzo approved these changes Aug 28, 2018

View reviewed changes

lukel97 changed the title ~~Use Brittany with hsimport when needed~~ [WIP]Use Brittany with hsimport when needed Sep 1, 2018

lukel97 added 3 commits November 4, 2018 22:13

Merge branch 'master' of https://github.com/haskell/haskell-ide-engine …

df6af9b

…into brittany-hsimport

Integrate LspFuncs into IdeM/IdeGhcM

6d8f669

This allows for plugins to now access config options It also streamlines the process of running the monads from Scheduler Should be expanded upon to move VFS into

Fix tests

e74dcc0

lukel97 added 3 commits November 5, 2018 00:06

Add func for VFS in IdeM

fc89420

Add config for formatting on import

82d20f0

Update test description

29d369f

lukel97 changed the title ~~[WIP]Use Brittany with hsimport when needed~~ Use Brittany with hsimport when needed Nov 5, 2018

lukel97 requested review from alanz and lorenzo November 5, 2018 00:51

lukel97 added type: enhancement type: refactor Refactor and tidy up internals. labels Nov 5, 2018

lukel97 force-pushed the brittany-hsimport branch from 4a2b618 to 9e0f196 Compare November 5, 2018 13:11

Fix config and add test

8c204cf

lukel97 force-pushed the brittany-hsimport branch from 9e0f196 to 8c204cf Compare November 5, 2018 14:13

Re-disable hlint in liquid tests

b5c11fd

lorenzo reviewed Nov 6, 2018

View reviewed changes

Move plugins and pidCache into IdeEnv

0082759

lukel97 added 2 commits November 7, 2018 15:33

Merge PluginDescriptor into PluginsIdeMonads

f4bdcda

Move getDiagnosticProvidersConfig there too

Merge branch 'master' of https://github.com/haskell/haskell-ide-engine …

c0dd689

…into brittany-hsimport

lukel97 merged commit 8514a5b into haskell:master Nov 11, 2018

lukel97 mentioned this pull request Nov 11, 2018

Investigate flaky prefix command test #933

Closed

alanz added this to the prehistory milestone Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Brittany with hsimport when needed #783

Use Brittany with hsimport when needed #783

lukel97 commented Aug 25, 2018 •

edited

Loading

lorenzo Aug 25, 2018

Gurkenglas commented Aug 25, 2018 •

edited

Loading

lorenzo Aug 25, 2018

lukel97 Aug 27, 2018

lorenzo Aug 27, 2018

lukel97 Aug 28, 2018

kritzcreek Sep 3, 2018

lukel97 commented Aug 28, 2018

alanz commented Oct 28, 2018

alanz commented Oct 28, 2018

lukel97 commented Nov 4, 2018

lukel97 commented Nov 5, 2018

lorenzo left a comment

lorenzo Nov 6, 2018

lukel97 Nov 6, 2018

lorenzo Nov 6, 2018

alanz Nov 7, 2018

lukel97 Nov 7, 2018 •

edited

Loading

lorenzo Nov 6, 2018

lukel97 Nov 6, 2018

lorenzo Nov 6, 2018

lukel97 Nov 6, 2018

lukel97 commented Nov 6, 2018

lukel97 commented Nov 10, 2018

lorenzo commented Nov 10, 2018

alanz commented Nov 10, 2018

alanz commented Nov 10, 2018

Use Brittany with hsimport when needed #783

Use Brittany with hsimport when needed #783

Conversation

lukel97 commented Aug 25, 2018 • edited Loading

Todo

Choose a reason for hiding this comment

Gurkenglas commented Aug 25, 2018 • 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

lukel97 commented Aug 28, 2018

alanz commented Oct 28, 2018

alanz commented Oct 28, 2018

lukel97 commented Nov 4, 2018

lukel97 commented Nov 5, 2018

lorenzo 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

Choose a reason for hiding this comment

lukel97 Nov 7, 2018 • 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

lukel97 commented Nov 6, 2018

lukel97 commented Nov 10, 2018

lorenzo commented Nov 10, 2018

alanz commented Nov 10, 2018

alanz commented Nov 10, 2018

lukel97 commented Aug 25, 2018 •

edited

Loading

Gurkenglas commented Aug 25, 2018 •

edited

Loading

lukel97 Nov 7, 2018 •

edited

Loading