Skip to content

Conversation

LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Mar 23, 2022

What changes were proposed in this pull request?

This pr enabled ImportOrder check of Java code

Why are the changes needed?

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GA

@LuciferYang LuciferYang changed the title [DONT MERGE] Enable ImportOrder check of Java code [DONT MERGE] Enabled ImportOrder check of Java code Mar 23, 2022
@LuciferYang
Copy link
Contributor Author

The ImportOrder check of Java is disabled now and there is a TODO:

spark/dev/checkstyle.xml

Lines 156 to 163 in 4817b01

<!-- TODO: 11/09/15 disabled - order is currently wrong in many places -->
<!--
<module name="ImportOrder">
<property name="separated" value="true"/>
<property name="ordered" value="true"/>
<property name="groups" value="/^javax?\./,scala,*,org.apache.spark"/>
</module>
-->

I try to re-enable it and change the rule as follows:

     <module name="ImportOrder">
            <!-- simple imports -->
            <property name="separated" value="true"/>
            <property name="ordered" value="true"/>
            <property name="groups" value="/^javax?\./,scala,*,org.apache.spark"/>
            <!-- static imports -->
            <property name="option" value="bottom"/>
            <property name="staticGroups" value=""/>
            <property name="sortStaticImportsAlphabetically" value="true"/>
            <property name="separatedStaticGroups" value="true"/>
        </module>

The no-static imports is consistent with scala code, the static imports is an independent group after no-static imports, and it sorted by alphabetically.

@srowen @dongjoon-hyun @HyukjinKwon I wonder if we need to complete this TODO ?

@LuciferYang LuciferYang marked this pull request as draft March 23, 2022 09:32
@LuciferYang
Copy link
Contributor Author

LuciferYang commented Mar 23, 2022

The example of following the ImportOrder check rule:

import java.io.IOException;
import java.nio.ByteBuffer;


import scala.Tuple2;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;

import org.apache.spark.network.buffer.ManagedBuffer;
import org.apache.spark.network.buffer.NioManagedBuffer;

import static org.apache.spark.network.shuffle.RetryingBlockTransferor.BlockTransferStarter;
import static org.junit.Assert.*;
import static org.mockito.Mockito.*;

@srowen
Copy link
Member

srowen commented Mar 23, 2022

I think this is mostly too much noise to bother with. Most imports are nearly-correctly-ordered and it matters little anyway. If there is a weaker check we can enforce (e.g. are imports grouped, if not ordered, correctly) that might be OK to try to enforce

@LuciferYang
Copy link
Contributor Author

So we can accept a weak rule similar to the following: imports are grouped and interior of each group can be disordered ?

@LuciferYang
Copy link
Contributor Author

Set order to false can ignore the order check

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Mar 23, 2022

@srowen for static imports, would you mind dividing them into a separate groups?

@LuciferYang
Copy link
Contributor Author

So we can accept a weak rule similar to the following: imports are grouped and interior of each group can be disordered ?

If this, still 180 files need to be changed :(

@srowen
Copy link
Member

srowen commented Mar 23, 2022

I see, it's still a big change then. I'm not sure it's worth it.

@LuciferYang
Copy link
Contributor Author

So should we remove this TODO, It seems unlikely to have a chance to finish it ...

@srowen
Copy link
Member

srowen commented Mar 23, 2022

We could, I also don't care much about the TODO statement either

@LuciferYang
Copy link
Contributor Author

We could, I also don't care much about the TODO statement either

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants