eclipse-cs icon indicating copy to clipboard operation
eclipse-cs copied to clipboard

Multiple source cleanups

Open Bananeweizen opened this issue 3 years ago • 3 comments

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 avatar May 28 '22 18:05 Bananeweizen

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 avatar May 29 '22 11:05 Bananeweizen

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

Bananeweizen avatar May 29 '22 12:05 Bananeweizen

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

rnveach avatar May 29 '22 18:05 rnveach

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 avatar Nov 12 '22 13:11 romani

@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.

https://github.com/checkstyle/eclipse-cs/blob/2cb3fb24be136882706b2dd4edd75ed31f9d6dae/config/checkstyle_sevntu_checks.xml#L15-L24

✔ ~/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 avatar Nov 12 '22 13:11 romani

@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 avatar Nov 12 '22 13:11 romani

@Bananeweizen , can we finish this PR ?

romani avatar Nov 13 '22 05:11 romani

I merging this PR as we found problem with Windows for patch filter https://github.com/checkstyle/patch-filters/issues/113 https://github.com/checkstyle/eclipse-cs/pull/395

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

romani avatar Nov 13 '22 14:11 romani

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 avatar Nov 13 '22 14:11 Bananeweizen