Issue #5337: Deprecate AutomaticBean#12213
Conversation
| */ | ||
| // -@cs[AbstractClassName] We can not break compatibility with previous versions. | ||
| @Deprecated(since = "10.3.4") | ||
| public abstract class AutomaticBean |
There was a problem hiding this comment.
#5337 (comment)
#5337 (comment)
Why can't we rename AutomaticBean to AbstractAutomaticBean and create a new AutomaticBean that extends AbstractAutomaticBean but is marked as deprecated?
With this type of make up, we won't need additional "ifs" and current code can just swap to AbstractAutomaticBean as both extend it.
There was a problem hiding this comment.
I see no problem. Never would have came up with this though.
There was a problem hiding this comment.
@rnveach it is. I'm trying to address the failing CI jobs and I will tag you then
There was a problem hiding this comment.
@rnveach I have kept the enum in the old class.
|
CI Failure: https://github.com/apache/maven-checkstyle-plugin/blob/627fa4f684866a579f2c105fcc1dbf3ed776daa8/src/main/java/org/apache/maven/plugins/checkstyle/AbstractCheckstyleReport.java#L63 |
|
It looks like this usage is preventing us from fully separating the classes like planned as we are completely reliant on this maven plugin and have not severed times with them. #11602 Unless there is any other ideas, |
|
Not sure why I think it automatically tries to use Should I also deal with
Ran |
I am not really a fan of checker, so I would concentrate on it's failures last. It looks like pitest is failing, so I would look at that first.
I am surprised some import statement is not needed for I do think one issue is there are 2
I don't know why the main report is not reporting anything, but do a scan on |
|
@rnveach Removed However, the |
|
@stoyanK7 The compilation error changed, it isn't the same issue, so you'll need to look at what we need to do to make it pass. All these compilation errors seem to mean a classpath is changing and the other projects we are compiling can't be changed until this PR is released, so you have to keep the classpaths the same. |
|
I am looking at no-exception-test.sh guava-with-google-checksWe have explicit import at AbstractCheckstyleReport.java#63 no-error-orekitWe have explicit import at CheckstyleViolationCheckMojo.java#65 These two and multiple other jobs that have the same errors are now working except Besides, there are already multiple explicit imports from |
The main difference I am seeing between these is that methods-distance extends Like I said, I don't understand how this is all working behind the scenes but this is appears to be how it is working. Feel free to test things out on your local to see how things are behaving and if a resolution can be made. As a fall back, you can try to add an import to the class and see if it will compile with this PR. In my Eclipse, it tells me its an unnecessary import and we have other static checks that also may flag it as unnecessary and prevent merging that fix.
Because the package name is different, so the imports have to be made explicit. All those classes are being instantiated and not extended which is making ===== The reason why it is "not working" anymore is because I also noticed that the Let me know if you have any other ideas. |
| // Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| /////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| package com.puppycrawl.tools.checkstyle.api; |
There was a problem hiding this comment.
as we create new class can we move it out of API package, we do not want implementations to be our API.
it might create bad cycle dependency of api package to implementation, but it is fine, it is only for migration period.
There was a problem hiding this comment.
Do we want it under just com.puppycrawl.tools.checkstyle?
IntelliJ is okay with this import for me when Cloning the repo and running it alone fails the same way. Am I missing something? |
|
Have you tried the other items before the "fallback" one? If the others are not working, please explain why. |
|
Currently testing them, the above one is an experiment as well. Will let you know when done. |
This is the command we run in CI. If you still have issues I would need to know what the failure is. |
|
@stoyanK7 ping |
|
On it. Apologies. |
| public DefaultLogger(OutputStream outputStream, | ||
| AutomaticBean.OutputStreamOptions outputStreamOptions) { | ||
| this(outputStream, OutputStreamOptions.values()[outputStreamOptions.ordinal()]); | ||
| } |
There was a problem hiding this comment.
@rnveach This is the enum conversion in action. We only need to do it here and in XMLLogger, so I think it is okay like that. Or maybe make some util function though it would be temporary until AutomaticBean gets removed. What do you think?
Taken from this StackOverflow answer.
Do you have any ideas on how to tackle the new surviving mutation:
Source File: "DefaultLogger.java"
Class: "com.puppycrawl.tools.checkstyle.DefaultLogger"
Method: "<init>"
Line Contents: "this(outputStream, OutputStreamOptions.values()[outputStreamOptions.ordinal()]);"
Mutator: "org.pitest.mutationtest.engine.gregor.mutators.NonVoidMethodCallMutator"
Description: "removed call to com/puppycrawl/tools/checkstyle/api/AutomaticBean$OutputStreamOptions::ordinal"
Line Number: 82
ordinal() is essentially swapped with 0 so we get this(outputStream, OutputStreamOptions.values()[0]);
There was a problem hiding this comment.
Fields that get set depending on value of outputStreamOptions aren't public so we cannot just check them and fail a test.
There was a problem hiding this comment.
Do you have any ideas on how to tackle the new surviving mutation:
I would check out #12341 , but I don't seeing regression being helpful for this case where the target of the regression is basically infrastructure related.
You will need to examine the code and the mutation being applied to understand how to kill it.
ordinal
This is such odd code.
There was a problem hiding this comment.
Made a temporary public method isCloseStream() to kill the mutation.
This is such odd code.
Okay, casting by shared enum value:
this(outputStream, OutputStreamOptions.valueOf(outputStreamOptions.name()));
Done.
|
Ran
I assume it is the enum like before? |
|
@Vyom-Yadav @nrmancuso Edit: I will update this with more info.
We have a line coverage drop, but not a mutation drop (least not failed with). So this is why we have "no new surviving mutations". For me, master says 640/640. The report downloaded from the CI does not have 538/544 for line coverage. It says 532/532 and 100%. The CI report has lost I can confirm CI behavior in my local. |
|
@stoyanK7 This looks like an issue with pitest. It doesn't report data on Please report issue to https://github.com/hcoles/pitest/issues/ so we can get their take and look into the class you changed and it's code coverage to show us it is still at 100%. Also please resolve other CI errors and anything left for this PR. Edit: Edit 2: Report doesn't list the remaining |
|
@rnveach Most of the checker errors are just old ones that are being moved to |
|
Just reminder that we need compatibility. We need to be sure that sevntu works fine. |
|
Sevntu fails because #12755 is waiting to get merged. |
|
I marked such violations as wontfix |
|
@stoyanK7 , please resolve conflict. |
|
@romani Can I resolve this conflict when this PR is approved? This conflict happens almost every day. |
romani
left a comment
There was a problem hiding this comment.
I see solution backward compatible , that is good.
items:
ok. |
romani
left a comment
There was a problem hiding this comment.
Ok to merge.
All reviewer are mandatory to finish review
Resolves #5337
First step of deprecating
AutomaticBeanand moving toAbstractAutomaticBean.