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

Option to disable import lens plugin #426

Closed
AlistairB opened this issue Sep 24, 2020 · 23 comments
Closed

Option to disable import lens plugin #426

AlistairB opened this issue Sep 24, 2020 · 23 comments
Labels
component: imports plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request

Comments

@AlistairB
Copy link
Contributor

AlistairB commented Sep 24, 2020

The import lens plugin is cool, however I believe it is not always useful and has some negative impact.

Why not always specify exact import lists?

I think specific import lists (or qualified imports) is great when importing from dependent libraries that have a lot of exports in a specific module.

However, when developing boring business applications with a large domain, you often want to break it down into a lot of very specific small types in small modules. This gives more type safety and also reduces recompilation time.

When you have a lot of modules with one thing in it, specifying import lists becomes burdensome and is essentially just repeating yourself. eg. import MyApp.Model.Person (Person(Person)).

What are the downsides of the ignoring the plugin?

It creates a lot of visual noise

image

It takes time to load

It takes about 3 seconds to load which moves the code down.


If this is a reasonable / desirable change I'd be happy to attempt it if I could get pointers on how to make it.

I think the other alternative which might work for me is a way to apply the plugin on all imports in one go. That might make it worth it to just always have specific import lists. Or both of these could be done.

@jneira
Copy link
Member

jneira commented Sep 24, 2020

I think a code action to apply all at once would be nice, it will solve the proble while adding a nice feature. But being able to disable it makes sense too.
Afaiu disable it will be easier though it would suppose:

  • create a new Boolean config field

data Config =
Config
{ hlintOn :: Bool
, diagnosticsOnChange :: Bool
, maxNumberOfProblems :: Int
, diagnosticsDebounceDuration :: Int
, liquidOn :: Bool
, completionSnippetsOn :: Bool
, formatOnImportOn :: Bool
, formattingProvider :: T.Text
} deriving (Show,Eq)

  • guard the code lens return result using that field

case mbMinImports of
-- Implement the provider logic:
-- for every import, if it's lacking a explicit list, generate a code lens
Just minImports -> do
let minImportsMap =
Map.fromList [ (srcSpanStart l, i) | L l i <- minImports ]
commands <- forM imports $ generateLens pId _uri minImportsMap
return $ Right (List $ catMaybes commands)
_ ->
return $ Right (List [])

  • you can get the config using LspFuncsas in

  • add the config field to vscode extension (as hlint)

https://github.com/haskell/vscode-haskell/blob/6e8e00c1cb3af98f34407fd7565e5430ebb909bd/package.json#L70-L76

@pepeiborra
Copy link
Collaborator

pepeiborra commented Sep 24, 2020

I have a code action coming soon. But I don't plan to add any option to disable the code lens within the plugin, since I believe this is better solved by HLS allowing users to select which plugins they want to use.

@jneira
Copy link
Member

jneira commented Sep 24, 2020

Well, you may want to enable it per project or depending on what module are you editing, no?
Disable/enable all code lenses could be another option but maybe you want them for some use cases and not for others.

@alanz
Copy link
Collaborator

alanz commented Sep 24, 2020

I recall a discussion on the hie issue tracker about carving our a namespace in the config, indexed by plugin ID, which would allow each individual plugin to provide config, in a composable and modular way.

See haskell/haskell-ide-engine#800. Perhaps we should revisit that?

And the config should be managed by the client, e.g vscode has the ability to set global or workspace settings. Emacs too.

@ndmitchell
Copy link
Collaborator

I was recently editing a file with lots of module imports, trying to edit the imports themselves, and having every other line be generated and with a lag made it really difficult. I would definitely like to disable this feature, but given its so prominent, I'd really like a code action to disable it rather than having to search around the docs, since its such an obvious thing to want. I definitely want all the other code lenses, so don't want to disable them for all.

I would really like to get this information in the hover information, as that's far less intrusive, and doesn't harm my editing lines experience.

@pepeiborra
Copy link
Collaborator

I don't believe hover is a better UI for this, since the point is to see what every import statement is pulling at a glance.

The lag will be gone in the next release of HLS, which will include the change to use stale data.

The best UI would diff the import statement with the code lens, and show the diff instead of the original line.

VSCode allows to customise settings per project. A simple HLS setting to select which plugins are active would go a long way. There is no need to search around the docs, the HLS VS Code extension settings are very easy to find and users are already familiar with this.

@jneira
Copy link
Member

jneira commented Sep 24, 2020

I have a code action coming soon. But I don't plan to add any option to disable the code lens within the plugin, since I believe this is better solved by HLS allowing users to select which plugins they want to use.

Agree that it would be the right, generic solution.

@alanz
Copy link
Collaborator

alanz commented Sep 24, 2020

I think it is useful, but not something you always want. Especially if you are not in the habit of explicitly listing the imports, which makes going anywhere near the import list a nightmare.

I think something like all of the following

  1. enable/disable via a configuration setting
  2. Present a lens once, at the top, to enable/disable the "full" version. It just says that there is potential work to be done.
    2.1 this has an action associated, that can flip it to the behaviour we see now
  3. Potentially make the activation if there is work to be done a code action, which could be limited to the specific import being considered.

This does assume a lot more interactivity with the code lens refresh than current clients do though. Perhaps something to investigate.

@ndmitchell
Copy link
Collaborator

The best UI would diff the import statement with the code lens, and show the diff instead of the original line.

Rust Analyzer shows a lot of text in grey that "could" have been written by the user but wasn't. That works better when refactoring than the code lenses. As an example of what we could do:

image

There is no need to search around the docs, the HLS VS Code extension settings are very easy to find and users are already familiar with this.

I strongly doubt that is true. When I wanted to know how to disable a feature in the Rust Analyzer I raised a GitHub ticket: rust-lang/rust-analyzer#6054. It's got a bunch of comments and thumbs up suggesting I wasn't the only one. There's also another near-identical bug someone else raised rust-lang/rust-analyzer#6053. Given a feature that "gets in the way", I think it should be easy to turn off from nearby. If that adjusts the settings, that would be great, but I don't want to have to learn how we subqualify plugins (or even that HLS has plugins!) in order to reduce clutter.

That said, my preference would be to rework the feature to be less intrusive. One possibility:

  • A single lens to expand all imports in a block - so if you have 10 adjacent import lines, you could expand them all in one code lens. Still allows people to follow conventions about import explicitly from other packages, but everything from locally.
  • Use the grey text to give people information about what they import from where, in a way that doesn't interfere with editing (at least in VS Code).

@pepeiborra
Copy link
Collaborator

Yeah, the grey text is exactly what I want. What magic is that?

I think my position is that HLS should empower both plugin writers and users. As a plugin writer I don't want to have to roll my own solution for toggling the plugin on/off. If the code lens for imports is too intrusive and we don't want to confuse new users, then have it disabled by default and let power users figure out how to enable it in the HLS settings page.

@wz1000
Copy link
Collaborator

wz1000 commented Sep 24, 2020

rust-analyzer seems to have a lot of custom code in its vscode extension: https://github.com/rust-analyzer/rust-analyzer/blob/master/editors/code/src/lsp_ext.ts

@alanz
Copy link
Collaborator

alanz commented Sep 24, 2020

rust-analyzer seems to have a lot of custom code in its vscode extension

Yes, I am pretty sure the inline lens rendering is not in the lsp spec (yet).

I think we should strive to stay standard, and not put huge burdens on the vscode client developers

@AlistairB
Copy link
Contributor Author

I also agree that a general mechanism to enable / disable plugins makes sense. That seems like a good focus for this particular issue. Although, I suspect the task is a bit too involved for me to develop having not contributed to HLS before.

@jneira jneira added component: plugins status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request labels Sep 25, 2020
@pepeiborra
Copy link
Collaborator

Code action added in #436

@ProofOfKeags
Copy link

It would also be nice to disable it on a per import basis. I always use -XNoImplicitPrelude, then roll my own. Having all Prelude imports be explicit (things like Functor/Monad etc.) seems silly.

@srid
Copy link
Contributor

srid commented Oct 9, 2020

It would also be nice to disable it on a per import basis. I always use -XNoImplicitPrelude, then roll my own. Having all Prelude imports be explicit (things like Functor/Monad etc.) seems silly.

Yup. Not only for Prelude, but also Prelude-esque imports. Like Reflex.Dom.Core.

I also agree that a general mechanism to enable / disable plugins makes sense.

For the above, it would be nice to have a more fire-grained configuration than that. So each plugin can (optionally) provide its own config type that the user can override. For the import lens plugin, taking the above into consideration, it would be something like:

data Config = Config 
  { ignoreImports :: [Import]
  } 

Which the user can ultimately configure in VSCode through Workspace or User settings JSON (or UI).

@pepeiborra
Copy link
Collaborator

PRs accepted and encouraged!

@jneira
Copy link
Member

jneira commented Feb 24, 2021

Now we can disable features per type (code lens f.e.) and plugin thanks to #513 so it would be doable now, setting in .vscode/settings.json:

{
    "haskell": {
        "plugin" : {
            "importLens" : {
                "codeLensOn" : false
            }
        }
    }
}

@AlistairB feel fre to comment or reopen if it does not work

@istathar
Copy link

Putting that snippet to my home directory didn't seem to work, but adding:

    "haskell.plugin.importLens.codeLensOn": false

in the ~/.config/Code/User/settings.json file (ie, Prefernces: Open Settings (JSON)) did indeed do the trick. Hooray!

@AlistairB
Copy link
Contributor Author

Thanks so much! Working for me too 👍

@valmirjunior0088
Copy link

valmirjunior0088 commented Sep 8, 2021

Doesn't seem to be working for me.

I have:

{
    ...
    "haskell.plugin.importLens.codeLensOn": false,
    "haskell.plugin.importLens.codeActionsOn": false,
    ...
}

But I still get:

image

@jhrcek
Copy link
Collaborator

jhrcek commented Sep 8, 2021

@valmirjunior0088 The code lens in your screenshot is produced by refireImports plugin. See this for how to disable it

@valmirjunior0088
Copy link

@jhrcek

Works! Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: imports plugin status: in discussion Not actionable, because discussion is still ongoing or there's no decision yet type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests