Skip to content
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

Multiple source cleanups #350

Merged
merged 11 commits into from
Nov 13, 2022
Merged

Conversation

Bananeweizen
Copy link
Collaborator

I was going to fix some deprecations, but doing so I noticed that the code would get littered with more and more duplicated catch-clauses due to newly introduced exception types in the non deprecated methods. Therefore I converted catch to multi-catch and ran some other cleanups. All changes were done fully automated, except for removing some no longer necessary @SupressWarning statements.

@Bananeweizen
Copy link
Collaborator Author

Can someone explain why the build failed? It looks like it checks all changed files for checkstyle violations. If so, the current EclipseCS sources have around 1400 violations, therefore any somewhat bigger change will fail this check.

@Bananeweizen
Copy link
Collaborator Author

This needs to be rebased after #351 has been fixed and the resulting checkstyle issues are also fixed.

@rnveach
Copy link
Member

rnveach commented May 29, 2022

Edit: I see now you discovered the issue and why you say there is a conflict from the issue created.

@romani
Copy link
Member

romani commented Nov 12, 2022

on this branch, on my local, with mvn install in root folder of repo, I also have no violations.
but lines are longer that 100 limit for sure.

@Bananeweizen , can you wrap that several cases in new commit ?

@romani
Copy link
Member

romani commented Nov 12, 2022

@Bananeweizen ,

Patch filter works on last commit changes.
on local history is linear, so patch using your last commit about infra: re-enable IllegalInstantiation check
in Travis, all commits are combined as one merge commit that is applied over master HEAD (it is non configurable feature of Travis) to not execute CI on old code and clearly know result after merge.

<module name="com.puppycrawl.tools.checkstyle.filters.SuppressionPatchFilter">
<property name="file" value="show.patch"/>
<property name="strategy" value="patchedline" />
</module>
<module name="TreeWalker">
<module name="com.puppycrawl.tools.checkstyle.filters.SuppressionJavaPatchFilter">
<property name="file" value="show.patch"/>
<property name="strategy" value="patchedline" />
</module>

✔ ~/java/github/checkstyle/eclipse-cs [Bananeweizen/sourceCleanups L|✚ 1] 
$ head show.patch 
diff --git a/config/checkstyle_checks.xml b/config/checkstyle_checks.xml
index f97284c..1a504b4 100644
--- a/config/checkstyle_checks.xml
+++ b/config/checkstyle_checks.xml
@@ -202,6 +202,15 @@
             <property name="ignoreSetter" value="true"/>
             <property name="setterCanReturnItsClass" value="true"/>
         </module>
+        <module name="IllegalInstantiation">
+            <property name="classes"

So Travis is right :).
you suppose to reproduce this problem is you checkout to each commit seprately and run "mvn install" on each of them

As you do massing update, I recommend you to fix violation in new commit, make CI happy, and keep merging with green CI.

@romani
Copy link
Member

romani commented Nov 12, 2022

@Bananeweizen , reminder one more time.
if there be situation with avalanche of demands from Checkstyle , please do not follow it, report it reviewers and we all can agree to merge with red CI. After merge, it might not be problem, if last commit is not touching problematic lines. So violations will stay in backlog and will not bother people until next update.


This CI failure is better to fix, it is few pressing of "Enter" key, it should take few minutes.

@romani
Copy link
Member

romani commented Nov 13, 2022

@Bananeweizen , can we finish this PR ?

@romani romani merged commit bc5c85d into checkstyle:master Nov 13, 2022
@romani
Copy link
Member

romani commented Nov 13, 2022

I merging this PR as we found problem with Windows for patch filter
checkstyle/patch-filters#113
#395

without such fix, it will be not easy to fix problems in such big update.
We will keep enforcement in next PRs.

@Bananeweizen
Copy link
Collaborator Author

Sorry for not coming back to this earlier. I had tried to debug into the patch filter to understand what's going on, but I failed miserably with that. Next time I'll try to clean up open PRs first.

@Bananeweizen Bananeweizen deleted the sourceCleanups branch November 13, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants