Closed olafurpg closed 5 years ago
In my experience, using local imports for types isn't that common; it's more DSLs for implicits that benefit from local imports. So a global import feature will probably be closer to most people's coding style.
Since one can always optimize going from (3) (place at end of global list) to (4) (optimize/order imports), why not start with (3)? Plus, the (4) extension could also be considered to be placed into scalafix
, since it's not only LSP-related.
I'd say that people who are used to IntelliJ way of things would expect, that auto import will simple use global imports, unless it will cause some name collision or the class belongs to a list of things that should have only their parent package imported - e.g. mutable.MutableMap
- and it will fully qualified names if if cannot figure things out.
For initial support I think it can be enough if autoimport adds import at the end of the global list if there is no conflict and use full qualified name name collided with something. It can be refined later on for starters some minimal non-breaking behavior would be much appreciated.
Thanks for the feedback!
My primary concern with option 3 is that it produces diffs that are not visible in the editor. Based on my experience with IntelliJ, these inserted imports frequently cause cryptic git conflicts and surprising diffs during code review.
With option 2 (local imports) the inserted imports are visible in the editor so the user observes them and can manually move them to the correct position.
I’d like to add that I’m annoyed by the IntelliJ behavior that always inserts the override
keyword even when the code overrides nothing but implements an abstract member. I think this is a bad practice because if that member becomes non-abstract in a future version your code will have no clue that it is now overriding a default implementation (which may not be the initial intent). Therefore I would suggest not inserting the override
keyword in case the member is abstract.
That's a good point @julienrf, I hadn't thought of that.
Ah interesting @julienrf, we actually have the opposite policy at work: we add override to make sure you get meaningful errors if the abstract member changes signature. If you don't, you get a giant error on the entire class implementation, but if you add override, the error stays local and you spot it quickly.
@gabro If the abstract member changes signature then you also get an error: “missing unimplemented members…”
I think we need an implements
keyword (or annotation) that would give you the same advantages of using override
, but without the drawback of unintentionally overriding a member. But this is off-topic…
You do, but the error now points to the entire class, which is hard to work with when using inline diagnostics.
@julienrf @gabro Thanks for the feedback! You both make good points IMO, I've moved this discussion over to a new issue https://github.com/scalameta/metals/issues/565
I just opened #570 which implements the "insert local import" solution. I agree with @jympa in https://github.com/scalameta/metals/issues/540#issuecomment-471482575 that it's more common to import types in the global import list and it would nice to investigate in the future how we could support that use-case.
@olafurpg would it be ok to open an issue to track the feature "adding imports to global import list", maybe as opt-in? I agree about the usefulness of seeing the change in the file but TBH the mental overhead of moving the line to the top on every new symbol is pretty awful for my workflow.
Tracking the issue would enable the community to discuss it further, maybe I can help implementing it too.
@mriehl I agree the manual step of moving local imports is annoying, I opened https://github.com/scalameta/metals/issues/675 to propose changing the default behavior to insert global imports.
It would be nice if completions could be used to override methods like here
In order to implement this feature we need to first come up with a solution for inserting imports for symbols that are not in scope.
Here is a summary of different approaches that we can take:
Insert fully qualified names
Before
After
Pros:
Cons:
java.nio.file.attributes.BasicFileAttributes
into import + short name is time consuming and cumbersomeInsert imports in nearest enclosing block
Before
After
Pros:
Cons:
Insert imports at bottom of global import list nearest enclosing block
Before
After
Pros:
Cons:
Insert imports at the correct place in the global import list
Before
After
Pros:
Cons: