-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
New Implementation for ConstructorsDeclarationGroupingCheck #14844
Conversation
60e9925
to
80d1427
Compare
made the implementation stateless as you suggested @nrmancuso |
I was facing this similar error in my other PR too: any idea how about how to solve it? Edit: I'm facing the same error on master branch too |
2351665
to
bc12147
Compare
I'm not able to think any way to kill this surviving mutation: https://github.com/checkstyle/checkstyle/actions/runs/8950200747/job/24585310558?pr=14844#step:7:22 Here is the Lines 88 to 90 in bc12147
I added this code based on OverloadMethodsDeclarationCheck's implementation. After digging into this Check, I found that it killed this mutation by using the following code in its input file: Lines 155 to 167 in e6051f2
I tried using something similar to kill the mutation but it didn't seem to work. I initially added that So should I remove that I'm hesitating to remove that |
No I didn't. Thanks for the suggestion, I will try this :) |
Generating the diff report here cuz I'm not able to run the command provided in the docs properly:
|
GitHub, generate report |
Unless you crash your current PR, you can't do this here as you haven't applied the mutation to the code. If you are having issues running regression locally, then I suggest starting a discussion and provide us details to assist you. |
Sorry I didn't saw your comment before... I pushed the changes with mutated code, I was thinking of re-generating the regression report with this mutated code and find if is there any difference between this mutated code report and the original code report....
Sure, here is the error I'm getting:
I'm using jdk 21, groovy 4 and windows 11. the error is showing java.exe not found at The
This was my command to generate the regression report:
I modified this command and made it a single line otherwise it was giving me this error when I did not modified it and used the original one provided in the doc:
|
GitHub, generate report |
Re-generating the report with mutated code as mentioned here:
Will also generate the report locally once the error is fixed. I thought it would be better to compare these 2 regression reports until the error I was getting locally is solved. |
I meant a GH discussion. Not in the PR thread.
Java home isn't the bin directory. It is the one above it. |
Done. Here is the link: #14861.
Fixed. thanks for the help. I was able to run that command but was getting some error. I've mentioned that in the discussion linked above. |
I analyzed both of these reports: #14844 (comment) #14844 (comment). One report was with the original code I pushed and other report was with the mutated code. They don't have any differences, they both have same violations. They both are same so there is no after affects of removing that Should I still generate the regression report locally or can I just follow pitest and remove the mutated code? @rnveach |
I would still like to see it to ensure it is being generated correctly. To be clear. Base branch is suppose to be what you want merged in this PR. Patch branch is suppose to be base branch plus the mutation applied. The only differences that show up should be only related to the mutation, nothing related to what you want merged in this PR normally. |
I guided you at #14749 (comment). I can see why we might want to also include OBJBLOCK in the tokens for this check (anonymous classes), but we should filter out the OBJBLOCKs that are not in anonymous classes. You will never pass pitest with the code at Lines 88 to 90 in bc12147
Additionally, as I mentioned at #14749 (comment), we can clean the implementation in this PR up (simplify logic, remove mutable variables) by creating a list of child ASTs first. |
4f2845c
to
e91d31c
Compare
Anonymous classes can't have constructors. Here is the oracle doc's reference: https://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html
list of child ASTs? Do you mean specifying the CLASS_DEF, ENUM_DEF, RECORD_DEF in the getRequiredTokens()? Here is your full comment:
I understood the first point but didn't quite got the other points, I have made the changes according to first point: Lines 70 to 77 in e91d31c
Here is how the check works currently: The check firstly gets the If the The error message indicates the line number from where the separation started: checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties Line 6 in e91d31c
Example:
Error messages:
Here the error message indicates line 6 which is The implementation looks good to me, the time complexity might have got improved after adding the only required DEFs in the |
I have changed the Check's implementation a little bit as mentioned here: #14844 (comment) But I generated the diff report before that, both branches had same violations. Here is the report if you want to take a look at it: https://zopsss.github.io/Diff-Report/ The current implementation and the implementation I used in the Edit: I've excluded elasticsearch & openjdk17 in the .properties file. I was not able to clone them because of my unstable internet connection |
GitHub, generate report |
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.
Item:
...ecks/coding/constructorsdeclarationgrouping/InputConstructorsDeclarationGroupingRecords.java
Outdated
Show resolved
Hide resolved
b1bbce3
to
f6ec2d7
Compare
The javac11 is failing, I'm getting this error in my other PR too |
@Zopsss , please rebase, it is fixed in master |
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.
Please try this:
config/checker-framework-suppressions/checker-lock-tainting-suppressions.xml
Show resolved
Hide resolved
Please remove second commit or squash commits together to single commit. Initial implementation will be stored in old PR forever, no need to keep it here. |
c6c3ed8
to
f1bf1f3
Compare
Done |
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 feel the stateful version of this check was better. I am not really going to look at this.
I see but other maintainers are agreeing on continuing using stateless version of this check. I guess it is better if we're able to make this check stateless. It would be really helpful if you also approve the new implementation. |
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 to merge.
I like Stateless Checks, it will be good performance ones we come to multithreaded checkstyle.
We need new report, as last report is for old code with different message(log approach).
GitHub, generate report |
Issue: #14726
Follow up of #14749
New implementation of the Check based on @nrmancuso's suggestions at: #14749 (comment)
The Check now flags all the constructors which got separated from the grouped constructors.
Here is the new error message:
https://github.com/Zopsss/checkstyle/blob/64e425b6c68fe9e03832b55bcff0e69b8bad1544/src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties#L6
Example:
The error message will look something like this:
Here line '6' means this line:
int x;
, at this line the constructors' separation started.The message will specify the line from which the constructors got separated. This will help users to locate the first line from where the separation started. If you have any suggestions about improving the error message then please let me know.
I've kept 2 separate commits, the first commit is the initial implementation of the Check, the second commit is the new changes in the implementation so I can easily copy these changes back to the main PR in future.
New module config: https://gist.githubusercontent.com/Zopsss/b3dabe65814aae54ff98c62f9fe54d3e/raw/6372644a5aa81acba09f1ddc607a79cd0b56de58/ConstructorsDeclarationGrouping.xml
Diff Regression projects: https://gist.githubusercontent.com/Zopsss/22adadb570e4deb95296917244c580b3/raw/29ea756eada8915637b76678db4f46048c198808/projects-to-test-on-for-github-action.properties