Skip to content

Introduce ErrorProne, fix compiler warnings #4807

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Introduce ErrorProne, fix compiler warnings #4807

wants to merge 2 commits into from

Conversation

scordio
Copy link
Contributor

@scordio scordio commented Apr 5, 2025 •
edited
Loading

This PR introduces ErrorProne, sets the maven-compiler-plugin to fail in case of compiler warnings, and fixes all the warnings.

This is a preparation for:

@scordio

This comment was marked as outdated.

Sign in to view
@scordio scordio marked this pull request as draft April 6, 2025 08:05
@scordio scordio changed the title Favor factory methods instead of double braces initialization Introduce ErrorProne, fix compiler warnings Apr 7, 2025
@scordio scordio force-pushed the cosmetic branch 2 times, most recently from 8eb3acc to 8a6ddaf Compare April 7, 2025 22:27
@fmbenhassine
Copy link
Contributor

This is fantastic! Thank you for doing that, @scordio !

I have a plan to re-work/optimise the project's build/release process for the next major release (see #4319 as well), and this goes in that same direction.

When this is not a draft PR anymore, please let me know and it should be welcome to merge.

@scordio scordio force-pushed the cosmetic branch 5 times, most recently from f0efc64 to 459933c Compare April 13, 2025 15:02
@scordio scordio force-pushed the cosmetic branch 17 times, most recently from 6349088 to 48ae732 Compare May 7, 2025 06:38
@scordio scordio mentioned this pull request May 9, 2025
Merged
@scordio scordio closed this May 9, 2025
@scordio scordio deleted the cosmetic branch May 9, 2025 23:28
@scordio scordio restored the cosmetic branch May 9, 2025 23:29
@scordio scordio reopened this May 9, 2025
@scordio scordio force-pushed the cosmetic branch 2 times, most recently from 4f4dbfa to ec33b20 Compare May 28, 2025 14:33
scordio
scordio commented May 28, 2025
View reviewed changes
Copy link
Contributor Author

@scordio scordio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fmbenhassine, I am finally ready with the changes!

I think there are a few flaky tests like:

  • SimpleStepFactoryBeanTests.testSimpleConcurrentJob
  • JdbcPagingItemReaderCommonTests

I couldn't figure out the root cause, but the failing ones are green with single executions in the IDE or when retrying a complete Maven build... if you have any hints, please let me know!

...tch-core/src/main/java/org/springframework/batch/core/repository/dao/JdbcJobInstanceDao.java Outdated
Comment on lines 59 to 63
@SuppressWarnings("unused")
private static final String STAR_WILDCARD = "*";

@SuppressWarnings("unused")
private static final String SQL_WILDCARD = "%";
Copy link
Contributor Author

@scordio scordio May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppressed the warnings here, but it might make sense to delete them instead.

...ructure/src/main/java/org/springframework/batch/item/file/transform/ConversionException.java
Comment on lines +23 to 24
@SuppressWarnings("unused") // FIXME no usage - should it be deprecated for removal?
public class ConversionException extends RuntimeException {
Copy link
Contributor Author

@scordio scordio May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment mentions, ConversionException is currently unused in the codebase.

Should this be deprecated for removal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will take care of that separately. Thank you for adding that "FIXME" for reference 👍

pom.xml Outdated
Comment on lines 189 to 227
-Xep:AlmostJavadoc:OFF
-Xep:BadInstanceof:OFF <!-- FIXME gh-4839 -->
-Xep:ByteBufferBackingArray:OFF
-Xep:ClassCanBeStatic:OFF
-Xep:CollectionUndefinedEquality:OFF
-Xep:DefaultCharset:OFF
-Xep:DirectInvocationOnMock:OFF
-Xep:DoNotCallSuggester:OFF
-Xep:EmptyCatch:OFF
-Xep:EqualsGetClass:OFF
-Xep:Finally:OFF
-Xep:FutureReturnValueIgnored:OFF
-Xep:HidingField:OFF
-Xep:ImmutableEnumChecker:OFF
-Xep:InlineMeSuggester:OFF
-Xep:InputStreamSlowMultibyteRead:OFF
-Xep:JavaTimeDefaultTimeZone:OFF
-Xep:JavaUtilDate:OFF
-Xep:JdkObsolete:OFF
-Xep:MissingSummary:OFF
-Xep:MixedMutabilityReturnType:OFF
-Xep:MutablePublicArray:OFF
-Xep:NonAtomicVolatileUpdate:OFF
-Xep:RedundantControlFlow:OFF
-Xep:ReferenceEquality:OFF
-Xep:StaticAssignmentInConstructor:OFF
-Xep:StaticAssignmentOfThrowable:OFF
-Xep:StaticMockMember:OFF
-Xep:StreamResourceLeak:OFF
-Xep:StringCaseLocaleUsage:OFF
-Xep:StringSplitter:OFF
-Xep:SynchronizeOnNonFinalField:OFF
-Xep:ThreadLocalUsage:OFF
-Xep:ThreadPriorityCheck:OFF
-Xep:TypeParameterUnusedInFormals:OFF
-Xep:UndefinedEquals:OFF
-Xep:UnnecessaryStringBuilder:OFF
-Xep:UnusedMethod:OFF
-Xep:UnusedVariable:OFF
-Xep:WaitNotInLoop:OFF
Copy link
Contributor Author

@scordio scordio May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded all the ErrorProne warnings that didn't have a straightforward solution.

I would suggest addressing them in separate PRs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's fine, we can address those in separate PRs.

@scordio scordio marked this pull request as ready for review May 28, 2025 18:13
scordio
scordio commented May 28, 2025
View reviewed changes
@scordio scordio force-pushed the cosmetic branch 2 times, most recently from e96e01d to aaf4582 Compare June 2, 2025 16:53
@scordio
Copy link
Contributor Author

scordio commented Jun 2, 2025

... and now the build is green! 😅

@fmbenhassine
Copy link
Contributor

This is a huge amount of work! Thank you very much for doing that, @scordio 👍

I know you had to rebase this several times due to the latest changes in the code base, really appreciated

Rebased, squashed and merged as 74975c5.

@fmbenhassine fmbenhassine added this to the 6.0.0-M1 milestone Jun 3, 2025
@scordio
Copy link
Contributor Author

scordio commented Jun 3, 2025

Thanks! I'll continue with #4673 in the upcoming days 🙂

@scordio scordio deleted the cosmetic branch June 3, 2025 23:14
@cppwfs cppwfs mentioned this pull request Jun 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmbenhassine fmbenhassine fmbenhassine left review comments

Assignees
No one assigned
Labels
in: build pr-for: enhancement
Projects
None yet
Milestone
6.0.0-M1
Development

Successfully merging this pull request may close these issues.
2 participants
@scordio @fmbenhassine