-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Remove unused Serializable instances #10783
Remove unused Serializable instances #10783
Conversation
These two classes are not serialized anywhere (anymore).
Quality Gate failedFailed conditions C Maintainability Rating on New Code (required ≥ A) See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Hi @Profpatsch, Thank you for your contribution and the thought put into this pull request! I appreciate your effort in addressing the identified concern. Considering the upcoming migration to Kotlin and Compose, it will likely address the concerns you've raised, making this change unnecessary in the long run. To focus our collective efforts optimally, I recommend: Additional feedback: I hope this perspective helps! Feel free to discuss any questions or concerns you might have. CC: @opusforlife2, @AudricV |
Hey snaik, good to see the reviews starting! Also, we get notifications, so no need to ping by username. I'm no dev, but for tiny code quality improvements like this, is it worth thinking about them too much before merging? As long as it's clear that there will be no regression, I mean. |
SG. Will avoid tagging by username.
While the code change seems small, it's essential to carefully consider its potential impact before merging, even in the absence of obvious regressions. Here's why:
|
Ehm, this is a pretty easy simplification that does not touch serialization. It just removes unused implementations. It compiles, it’s good to go. |
Unfortunately it's not true that if it compiles, then it works. This is what I get when I start the player. I agree with @snaik20 , it's better to avoid risking to introduce regressions if the improvements to the code are this small. But thank you anyway!
|
Is it seriously resolving instances at runtime? what a horrible design |
These two classes are not serialized anywhere (anymore).
What is it?
Due diligence