Lift-4: remove simple warnings#2027
Merged
farmdawgnation merged 2 commits intolift:lift-4.0from Nov 21, 2024
Merged
Conversation
pabloazul
reviewed
Nov 11, 2024
Member
farmdawgnation
left a comment
There was a problem hiding this comment.
Looks good overall. I'll want to confirm test passage locally before we merge, but I left you some comments!
Thank you. This really is thankless work and I deeply appreciate it.
cf4afd7 to
e71876b
Compare
farmdawgnation
approved these changes
Nov 21, 2024
Member
farmdawgnation
left a comment
There was a problem hiding this comment.
This looks good to me, thank you for this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal of that PR is to decrease the number of compilation warnings in branch
lift-4.0so that it will be easier to see real problem when real changes will be introduced.There is 3 categories of warnings:
1/ Changed
Type ascription
Scala 3 wants A LOT of type ascription. It clutters a lot sources, but well, that horse is far now.
I've added some, millions remain.
adding ()
=> Add a lot of ()
function call of method without ()
Method without
()can't be transformed into function withmethod _, we must now use() => method.Uppercase for numeric long
=> use
L###
+on char/any deprecatedUse
.toString, or string interpolation depending what is the most readableorg.scalatest
=> Use the provided import
Array: _*
=> call
.toseq()### longToTimeSpan
=> add
TimeSpan(...)newInstance
Use what is advised in
newInstance:Procedure syntax
=> use
: Unit =A pure expression does nothing in statement position
Remove the expression (each time in tests)
2/ Not changed, potential real bugs
There is several real warning that can be important and should be corrected.
Shadowing a parent nested class is deprecated
Not sure what we can do for that one. I don't know if the support is removed, but if so, it will likely change an API.
Perhaps can be
nowarned until real port to a version of scala not supporting it anymore.Outer reference not checked at runtime
LiftScreen and SHtml produce:
It might be a bug if used in pattern matching, or a useless warning against a bug of scalac that could be ignore, see:
match may not be exhaustive.
There is several case of non exhaustive match that should be investigated.
a == null is always false
Not sure if it reveals something important, I let it as it was
3/ Not changed, need understanding of Lift intent
### DateMidnight
### URL
This is only for JDK 20+, change will need to work on 11+. Since it's central to Lift, I didn't want to pass it in a general warning correction PR.
### TimeHelpersSpec
There is several deprecation in that class, I'm not sure about what it tests so I let them like that.
Stream
Stream is often use to avoid duplicate evaluation, and I'm not sure about
LazyListsemantic, need to be checked by someone with more knowledge than me in Lift intent.