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

Fix/Remove some deprecated warnings #3506

Closed
wants to merge 7 commits into from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Oct 24, 2024

This PR removes some (not yet all) deprecated warnings during compilation.

I've organized this into separate commits.

There should be no effective code changes in this PR

Roland

@rbygrave
Copy link
Member

rbygrave commented Oct 24, 2024

So generally I only do the order() -> orderBy() changes when we remove the deprecated method in question. The reasons are that it keeps our test coverage on the deprecated methods, it shows we actually had test coverage in the first place, the PR that removes the deprecated method provides examples of the impact [for people looking at the PR because they didn't look at the warnings and now their code doesn't compile].

So to me these should have been separate PRs and then the ones like order() -> orderBy() only get merged with the actual removal of the deprecated method. Removing deprecated methods is significant and so they deserve their own PR. Plus having their own git issue title makes them more searchable via google etc.

I realise this is a "grey area" and I appreciate the effort but I don't want to merge this PR in this form.

@rPraml
Copy link
Contributor Author

rPraml commented Oct 24, 2024

Can you cherry pick that commit: c379abd
Or would it be better if I do another PR?

@rPraml
Copy link
Contributor Author

rPraml commented Oct 25, 2024

for people looking at the PR because they didn't look at the warnings and now their code doesn't compile

Haha, I know this problem too good. 🤣🤣

We've adjusted our compiler settings few months ago, that build will fail, if you use a deprecated forRemoval code. The only way is to mark that code part with @SupressWarinings("removal")

I've currently the issue, that an ebean build prints out hundreds of warnings on a build (Starting with 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-javadoc-plugin is missing - javadoc generation also fails on my machine)

I would be willing to support you a little here and clear these things up. That's why I startet with the (at least for me) most annoying thing, that IntelliJ underlines order() in red in every second test class.

I've exctracted the orderby thing in a separate PR: #3508 and I will close this for now

@rPraml rPraml closed this Oct 25, 2024
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.

2 participants