-
Notifications
You must be signed in to change notification settings - Fork 407
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
checkstyle rule for java import statements #561
Conversation
brenuart
commented
Jul 7, 2021
- update checkstyle rule to assert proper ordering of java import statements
- update source files to conform with the decided ordering
71ce630
to
6cc21ee
Compare
(I squashed the commits locally then force push - may not be very wise tho... maybe I should let you squash them at the time you merge the PR?) |
@@ -36,7 +36,14 @@ | |||
<module name="AvoidStarImport"/> | |||
<module name="IllegalImport"/> <!-- defaults to sun.* packages --> | |||
<module name="RedundantImport"/> | |||
|
|||
<module name="ImportOrder"> | |||
<property name="groups" value="java,javax,net.logstash"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove javax
I think one group for java and javax without a separator between is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I can have checkstyle putting both java.*
and javax.*
in the same group with a regular expression - but IDE like Eclipse doesn't like it :-(
The project isn't using any javax.*
stuff anyway so, either:
- I remove the
javax
group and let them fall into the "other" category - I keep it asis with possibly two "java" groups at the top - "just in case..." (project is not using any javax stuff anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove javax.
A lot of that namespace is slowly being replaced with jakarta anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong... we have a few javax.net.ssl.*
stuff... removing the javax
group gives something like this:
import java.io.BufferedOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketTimeoutException;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Formatter;
import java.util.List;
import java.util.Optional;
import net.logstash.logback.appender.destination.DelegateDestinationConnectionStrategy;
import net.logstash.logback.appender.listener.TcpAppenderListener;
import net.logstash.logback.encoder.SeparatorParser;
import net.logstash.logback.encoder.StreamingEncoder;
import ch.qos.logback.core.net.ssl.ConfigurableSSLSocketFactory;
import ch.qos.logback.core.net.ssl.SSLConfigurableSocket;
import com.lmax.disruptor.EventHandler;
import com.lmax.disruptor.LifecycleAware;
import com.lmax.disruptor.RingBuffer;
import javax.net.SocketFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocket;
import javax.net.ssl.SSLSocketFactory;
Do you still like it? Personally I'm more in favour of a javax
group at the top below java
.. matter of taste ;-)
Up to you...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I was thinking it was prefix based, and that java would match java and javax. But since that's not the case, we can keep it.
I'm generally not a fan of force pushing after comments have already been made on the review. Seeing the diff of new commits help understand how the comments were resolved. However, feel free to force push if there aren't any comments on the review yet. |
Related issue: logfellow#556
- static imports go on top in their own separate group (not mixed with other groups) - ban unused imports
a82dba8
to
06415e8
Compare