Conversation
olamy
left a comment
olamy
left a comment
There was a problem hiding this comment.
some changes are breaking backward compat.
| boolean reuseForks, | ||
| @Nonnull Platform pluginPlatform, | ||
| @Nonnull ConsoleLogger log ) | ||
| AbstractClasspathForkConfiguration( @Nonnull Classpath bootClasspath, |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
There was a problem hiding this comment.
and what about users/project outside of this project? There are so many usage of maven by tools (ide etc...) so we have no idea who is using those classes. They are public so keep them public as we don't know... Backward compatibility for opensource library/project with large audience is a pain but we must maintain it... and it's more important than a checkstyle failure.....
There was a problem hiding this comment.
I don't buy that. Those are some internal classes, not a part of any API. I fact I don't think that any part of surefire-plugin is a API. Is there any real projects, that rely on surefire internal classes?
I think that eventual API classes and interfaces should be marked strictly as API and kept backward compatible. But not internal classes.
There was a problem hiding this comment.
I guess you're right sorry I didn't check class declaration. so yes that will work.
So let's go for checkstyle driven development..... :-)
| private final Thread inputStreamCloserHook; | ||
|
|
||
| public CloseableCloser( int jvmRun, Closeable... testProvidingInputStream ) | ||
| CloseableCloser( int jvmRun, Closeable... testProvidingInputStream ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
| private int c2; | ||
|
|
||
| public EncodingOutputStream( OutputStream out ) | ||
| EncodingOutputStream( OutputStream out ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
|
|
||
| @SuppressWarnings( "checkstyle:magicnumber" ) | ||
| public Utf8RecodingDeferredFileOutputStream( String channel ) | ||
| Utf8RecodingDeferredFileOutputStream( String channel ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
| * @param delegate a target | ||
| */ | ||
| public ClassLoaderProxy( Object delegate ) | ||
| ClassLoaderProxy( Object delegate ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
| private Runner filteredRunner; | ||
|
|
||
| public FilteringRequest( Request req, Filter filter ) | ||
| FilteringRequest( Request req, Filter filter ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
| { | ||
| private final String value; | ||
|
|
||
| public IgnoredWithUserError( String value ) |
There was a problem hiding this comment.
why removing public constructor this is not backward compat???? if checkstyle complains about this so remove checkstyle usage as it's useless....
There was a problem hiding this comment.
This constructor wasn't used outside the package. Intellij also confirms it, so checkstyle reports it properly. Also keep in mind that tests passes. If it not being used, access can be safly lowered to package level.
3 participants
Current master do not builds!
It's due to incompatibility of checkstyle plugin and couple of checkstyle violations.
I've bumped maven-parent to latest version so plugins are now modern and also fixed those checkstyle violations so that project can be build now.
BTW. Consider to enable CI build. This will prevent this happening in future. There is a
.travis.ymlfile already, so 🙏