square / java-code-styles

IntelliJ IDEA code style settings for Square's Java and Android projects.
2.95k stars 769 forks source link

Concise inner class imports #75

Closed thomasmahoney closed 2 years ago

thomasmahoney commented 4 years ago

(I haven't used IntelliJ for that long and am far from an expert; apologies in advance if I'm missing something obvious or any part of this doesn't make sense!)

Hi, just wanted to suggest adding the INSERT_INNER_CLASS_IMPORTS option here. This maps to the "Insert imports for inner classes" checkbox IntelliJ setting in Editor > Code Style > Java > Imports tab.

With this enabled, and assuming a class like this:

public class Foo {
  public static class Bar {
  }
}

You can write Ba and press ⌥+Space to autocomplete and auto-import Bar.

With the flag disabled, you'll end up with Foo.Bar. That outer class qualifier can reduce ambiguity, but I'd argue that the majority of the time, you won't need to refer to multiple Bar types within a single file. When you do need this, it's simple to achieve the desired Foo.Bar outcome by autocompleting both fragments (e.g. type Fo -> ⌥+Space to import Foo -> type .Ba -> ⌥+Space to import Bar), or by writing Foo.Ba and autocompleting only the inner class if Foo is already imported.

On the other hand, with this option disabled, there's no equally-simple way (at least as far as I've found) to automatically perform the static import and enable unqualified references to Bar. It can be done with another ⌥+Space keypress to select "Add Import for...", but that's a second step that has to be done once per imported inner class per file. So, since enabling this option makes it easy to write code in both styles (either referring to OuterClass.InnerClass or to the unqualified InnerClass directly), while disabling this option makes the latter option difficult, I think enabling this option is a better default.

egraham-square commented 4 years ago

but that's a second step that has to be done once per imported inner class per file

And you have to remember to remove the original import of the outer class. So, it's two additional steps most of the time.

thomasmahoney commented 3 years ago

I noticed that IntelliJ auto-refactoring operations (such as "Change Signature") would automatically static-import inner class types that previously were qualified with their outer class type. This is problematic in the case of Builder types specifically, because most Square Java code deals with dozens of different top-level class types with associated Builder inner types (stemming mostly from our protoc-generated classes). Static-importing these is undesirable because it creates ambiguity and diminishes readability (i.e. Payment.Builder is preferable to Builder).

I've updated the PR to specifically prevent inner class imports for Builder types.