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

ImportOrder: Check that import groups aren't separated internally #2143

Closed
robertwhitebit opened this Issue Sep 8, 2015 · 14 comments

Comments

Projects
None yet
6 participants
@robertwhitebit

robertwhitebit commented Sep 8, 2015

Currently, the ImportOrder check ensures that imports are grouped in a specific order. These groups shouldn't be separated internally as well.

e.g.

import com.puppycrawl.tools.checkstyle.api.Check;

import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

The empty line between Check and DetailAST should be removed. Automatically importing classes will never add such a newline. The Android Studio formatter also removes the empty line.

@romani romani added the approved label Sep 8, 2015

@sshashank124

This comment has been minimized.

Show comment
Hide comment
@sshashank124

sshashank124 Sep 8, 2015

Until what point should they be grouped. Would you say that these two statements should also be grouped or only the innermost ones:

import org.apache.commons.beanutils.ConversionException;
import org.apache.commons.lang3.StringUtils;

sshashank124 commented Sep 8, 2015

Until what point should they be grouped. Would you say that these two statements should also be grouped or only the innermost ones:

import org.apache.commons.beanutils.ConversionException;
import org.apache.commons.lang3.StringUtils;
@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Sep 8, 2015

Contributor

@sshashank124, I believe that we should just not allow not explicitly allowed newlines in imports to match major IDEs formatting/optimise imports rules

Contributor

mkordas commented Sep 8, 2015

@sshashank124, I believe that we should just not allow not explicitly allowed newlines in imports to match major IDEs formatting/optimise imports rules

@robertwhitebit

This comment has been minimized.

Show comment
Hide comment
@robertwhitebit

robertwhitebit Sep 8, 2015

@sshashank124: This is a good question!
@mkordas: Exactly.

I tested it with the Eclipse and Android Studio (IntelliJ) formatter. You can configure how and at what level you define groups.
If you're using "java, com" then the formatter of the tested IDEs will group at the first level of the import.

import java.io.IOException;

import com.oracle.net.Sdp;
import com.oracle.nio.BufferSecretsPermission;

Inserting a newline between the two com imports and running the formatter/optimize imports will always remove the newline automatically.

However, if you are using something like "java, com.oracle.nio, com.oracle.net" then the result of the formatter is:

import java.io.IOException;

import com.oracle.nio.BufferSecretsPermission;

import com.oracle.net.Sdp;

Removing the newline between the com imports and running the formatter/optimize imports will always readd the newline.

robertwhitebit commented Sep 8, 2015

@sshashank124: This is a good question!
@mkordas: Exactly.

I tested it with the Eclipse and Android Studio (IntelliJ) formatter. You can configure how and at what level you define groups.
If you're using "java, com" then the formatter of the tested IDEs will group at the first level of the import.

import java.io.IOException;

import com.oracle.net.Sdp;
import com.oracle.nio.BufferSecretsPermission;

Inserting a newline between the two com imports and running the formatter/optimize imports will always remove the newline automatically.

However, if you are using something like "java, com.oracle.nio, com.oracle.net" then the result of the formatter is:

import java.io.IOException;

import com.oracle.nio.BufferSecretsPermission;

import com.oracle.net.Sdp;

Removing the newline between the com imports and running the formatter/optimize imports will always readd the newline.

@sshashank124

This comment has been minimized.

Show comment
Hide comment
@sshashank124

sshashank124 Sep 8, 2015

So then does it make sense to leave the level of grouping up to the individual or were you thinking of a predefined set of grouping rules

sshashank124 commented Sep 8, 2015

So then does it make sense to leave the level of grouping up to the individual or were you thinking of a predefined set of grouping rules

@robertwhitebit

This comment has been minimized.

Show comment
Hide comment
@robertwhitebit

robertwhitebit Sep 8, 2015

Hopefully I didn't misinterpret the question. 😟

The level of the grouping is already up to the user. You can set the groups property to e.g. "java,com.oracle.nio,com.oracle.net". So the second code snippet in the previous post is handled by checkstyle already.

Imo, the only thing that has to be done on the user side, is, to add a flag (true/false) if it's allowed to separate these groups internally.

So, I was just thinking of a flag. 😉

robertwhitebit commented Sep 8, 2015

Hopefully I didn't misinterpret the question. 😟

The level of the grouping is already up to the user. You can set the groups property to e.g. "java,com.oracle.nio,com.oracle.net". So the second code snippet in the previous post is handled by checkstyle already.

Imo, the only thing that has to be done on the user side, is, to add a flag (true/false) if it's allowed to separate these groups internally.

So, I was just thinking of a flag. 😉

@sshashank124

This comment has been minimized.

Show comment
Hide comment
@sshashank124

sshashank124 Sep 8, 2015

Ah I see. My bad. Yeah that's a good idea. So now, will someone delegate this to someone to implement if it is finally decided to be added?

sshashank124 commented Sep 8, 2015

Ah I see. My bad. Yeah that's a good idea. So now, will someone delegate this to someone to implement if it is finally decided to be added?

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Sep 8, 2015

Contributor

@sshashank124, anyone can take it and submit a PR. All contributions are welcome, also from you guys :)

Contributor

mkordas commented Sep 8, 2015

@sshashank124, anyone can take it and submit a PR. All contributions are welcome, also from you guys :)

@sshashank124

This comment has been minimized.

Show comment
Hide comment
@sshashank124

sshashank124 Sep 8, 2015

@mkordas, nice. I don't know enough about checkstyle at the moment to contribute. Definitely on future issues

sshashank124 commented Sep 8, 2015

@mkordas, nice. I don't know enough about checkstyle at the moment to contribute. Definitely on future issues

@robertwhitebit

This comment has been minimized.

Show comment
Hide comment
@robertwhitebit

robertwhitebit Sep 8, 2015

An interesting case is, separating a group by a comment. Like

import com.puppycrawl.tools.checkstyle.api.Check;
// import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

Eclipse and IntelliJ move or remove them. See sevntu-checkstyle/sevntu.checkstyle#373 for more details. I think that in these cases, the formatter does more than it should. But I just wanted to share this information here as well.

So just checking for empty lines between groups should be sufficient for now.

robertwhitebit commented Sep 8, 2015

An interesting case is, separating a group by a comment. Like

import com.puppycrawl.tools.checkstyle.api.Check;
// import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

Eclipse and IntelliJ move or remove them. See sevntu-checkstyle/sevntu.checkstyle#373 for more details. I think that in these cases, the formatter does more than it should. But I just wanted to share this information here as well.

So just checking for empty lines between groups should be sufficient for now.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 29, 2016

Member

@linelect , can you help us to resolve this issue?

Member

romani commented Oct 29, 2016

@linelect , can you help us to resolve this issue?

linelect added a commit to linelect/checkstyle that referenced this issue Nov 11, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 17, 2016

Issue #2143: Check that import groups aren't separated internally. Ed…
…it messages resource

and ImportOrderCheck (cyclomatic complexity was 13)

linelect added a commit to linelect/checkstyle that referenced this issue Nov 17, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 17, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 17, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 26, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 26, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 26, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 27, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 27, 2016

linelect added a commit to linelect/checkstyle that referenced this issue Nov 28, 2016

romani added a commit that referenced this issue Nov 28, 2016

@romani romani changed the title from Check that import groups aren't separated internally to ImportOrder: Check that import groups aren't separated internally Nov 28, 2016

@romani romani added the bug label Nov 28, 2016

@romani romani added this to the 7.4 milestone Nov 28, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 28, 2016

Member

fix is merged

Member

romani commented Nov 28, 2016

fix is merged

@romani romani closed this Nov 28, 2016

@jonmbake

This comment has been minimized.

Show comment
Hide comment
@jonmbake

jonmbake Jan 19, 2018

Contributor

@romani Shouldn't this only be enforced when ImportOrder. separated=true?

I am attempting to update from CheckStyle 6.14.1 to 8.7. I have the following CheckStyle rule specified:

<module name="ImportOrder">
    <property name="separated" value="false"/>
</module>

When running 6.14.1 CheckStyle, there are no violations. When running 8.7, there are hundreds of "Extra separation in import group before" violations. For example,

import javax.naming.NamingException;

import org.slf4j.Logger;

public class Foo {
}

Passed in 6.14.1, but now fails. This seems like a regression to me.

Contributor

jonmbake commented Jan 19, 2018

@romani Shouldn't this only be enforced when ImportOrder. separated=true?

I am attempting to update from CheckStyle 6.14.1 to 8.7. I have the following CheckStyle rule specified:

<module name="ImportOrder">
    <property name="separated" value="false"/>
</module>

When running 6.14.1 CheckStyle, there are no violations. When running 8.7, there are hundreds of "Extra separation in import group before" violations. For example,

import javax.naming.NamingException;

import org.slf4j.Logger;

public class Foo {
}

Passed in 6.14.1, but now fails. This seems like a regression to me.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 19, 2018

Member

@jonmbake , please open new issue, smth is brocken.
if you represent opensource code please provide link to project and how to run checkstyle - we will recheck that after fix you have no problems and we will add you to CI process to avoid regression.

Member

romani commented Jan 19, 2018

@jonmbake , please open new issue, smth is brocken.
if you represent opensource code please provide link to project and how to run checkstyle - we will recheck that after fix you have no problems and we will add you to CI process to avoid regression.

@robertwhitebit

This comment has been minimized.

Show comment
Hide comment
@robertwhitebit

robertwhitebit Feb 3, 2018

@linelect, Awesome! Thank you very much for fixing it! 😃
And, as always, also a big thanks to the Checkstyle team!

robertwhitebit commented Feb 3, 2018

@linelect, Awesome! Thank you very much for fixing it! 😃
And, as always, also a big thanks to the Checkstyle team!

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