Skip to content
This repository has been archived by the owner on Feb 4, 2025. It is now read-only.

Checkstyle rule enforcing fully qualified imports for Store nested classes #755

Merged
merged 4 commits into from
Apr 17, 2018

Conversation

aforcier
Copy link
Contributor

We've been favoring using fully qualified imports for inner classes of Stores (e.g, FetchPostsPayload instead of PostStore.FetchPostsPayload), but there's no checkstyle rule and AS's default behavior is not to add the fully qualified import.

This PR adds a checkstyle rule that flags any case of inner/nested classes being used without a fully qualified import, in an effort to keep things consistent and avoid having to point it out in PR reviews.

Initially I had also added a setting in the shared configs that would make AS use specific imports by default. But, while this works well for FluxC, I'm not sure if it's behaviour we want to use everywhere. The reason this makes sense for stores is that the inner classes tend to be errors, error types, payloads, and OnChanged events, all of which have pretty standard and clear naming. Some non-FluxC things though, like LinearLayout.LayoutParams params, are less clear when a specific import is used (several classes define LayoutParams).

So my proposal is to add the checkstyle rule to keep inner store imports clear, but not modify the default behavior. This might be a little annoying though. It depends how strongly we feel about keeping the style consistent (to me, it's automatic to hit Alt+Enter to use the more specific import, but the immediate checkstyle violation may be a slight nuisance when first using FluxC).

Would love some thoughts on this - @maxme, @oguzkocer in particular.

(Just for reference, the auto-import settings that can be enabled are in Preferences > Editor > Code Style > (Java > Imports > Insert imports for inner classes AND Kotlin > Imports > Other > Insert imports for nested classes).)

@aforcier aforcier added the Core label Mar 15, 2018
@oguzkocer
Copy link
Contributor

@aforcier I love this PR, thanks a lot for doing this. It was something that was bugging me a lot, but I also found it hard to bring up in each review.

I am actually OK with either alternative because the specific import is not that common and even if AS imports it, the developer can still opt to remove the specific import (which would be the reverse of this). However, as you said, doing the Alt+Enter for a checkstyle error is super easy to do as well. I am just glad to see something in place for the issue.

I'll defer to you and @maxme for the decision, but I am happy to do the PR review and check things in Windows once the decision is made.

@aforcier aforcier force-pushed the feature/inner-class-imports branch from 5b7267f to 8875bfe Compare March 19, 2018 14:03
@aforcier aforcier force-pushed the feature/inner-class-imports branch 2 times, most recently from 008b996 to 8ff7f50 Compare April 3, 2018 15:29
@maxme maxme self-assigned this Apr 16, 2018
@aforcier
Copy link
Contributor Author

So my proposal is to add the checkstyle rule to keep inner store imports clear, but not modify the default behavior. This might be a little annoying though. It depends how strongly we feel about keeping the style consistent (to me, it's automatic to hit Alt+Enter to use the more specific import, but the immediate checkstyle violation may be a slight nuisance when first using FluxC).

Since making this comment I'm changing my mind a bit, I think modifying the default behavior to auto-use nested class imports makes sense. It's pretty annoying in Kotlin to add them manually sometimes - the IDE is a bit buggy sometimes and won't suggest the right import option when pressing Alt+Enter on a reference, and I've had to manually add the import to the list (doesn't happen if you turn on auto-nested class imports).

So I guess @oguzkocer and I are both now tending towards using auto-imports - what do you think @maxme? I'll update the PR to include that unless you're strongly against.

@aforcier aforcier force-pushed the feature/inner-class-imports branch from 8ff7f50 to 756d7c9 Compare April 16, 2018 21:12
@maxme
Copy link
Contributor

maxme commented Apr 17, 2018

@aforcier do you want to add

    <option name="INSERT_INNER_CLASS_IMPORTS" value="true" />

To the Project.xml file?

@aforcier
Copy link
Contributor Author

do you want to add

  <option name="INSERT_INNER_CLASS_IMPORTS" value="true" />

To the Project.xml file?

Yep, and also

<JetCodeStyleSettings>
  ...
  <option name="IMPORT_NESTED_CLASSES" value="true" />
</JetCodeStyleSettings>

(for Kotlin). Added in 2a74860.

@maxme
Copy link
Contributor

maxme commented Apr 17, 2018

:shipit:

@maxme maxme merged commit 559ae8e into develop Apr 17, 2018
@maxme maxme deleted the feature/inner-class-imports branch April 17, 2018 12:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants