-
Notifications
You must be signed in to change notification settings - Fork 92
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
Support Scala 2.13.12 #789
Conversation
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #789 +/- ##
==========================================
- Coverage 87.22% 85.61% -1.62%
==========================================
Files 141 141
Lines 1527 1529 +2
Branches 254 258 +4
==========================================
- Hits 1332 1309 -23
- Misses 195 220 +25
☔ View full report in Codecov by Sentry. |
I am only wondering about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commits except mine and they LGTM
|
||
val isScala213 = shortScalaVersion == "2.13" | ||
val isScala213: Boolean = shortScalaVersion == "2.13" | ||
val isScala21312: Boolean = scalaVersion == "2.13.12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if instead of string comparison, we'll try to use major/minor/patch versions as numbers?
Something like this, but ensuring that the split is 'safe':
val (major, minor, patch) = scalaVersion.split('.').map(_.toInt)
val isScala213: Boolean = major == 2 && minor == 13
val isScala21312OrLater: Boolean = isScala213 && patch >= 12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! We'd welcome your PR with this small change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to become a gotcha for new 2.13 versions. Probably can find some time this week to raise this as an MR.
How lenient do we want to be here when we can't parse the string?
- Fail fast (as scapegoat is full cross-built, might not be the worst)
- Fail per component (2.13.231-snapshot could still be understood as 2.13.??? so the is 2.13 check would still work)
- Fallback to something (not sure there's a sane default considering 2.12/2.13 though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think what @saeltz mentioned here would be a good amendment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#826 raised here :)
@mccartney, are you in for a new release? |
Forgot to mention here, but |
Fixes #780.
Fixes #754.
Closes #781.
Multiple things were changed:
context
used twice in the same scope in inspectionspostTypeTraverser
in inspectionsval inspections
in testsUnnecessaryConversion
inspection on Scala 2.13.12FeedbackTest
to compile with new property inStoreReporter.Info
fix
on the entire codebase (it failed somehow in GitHub Actions after my changes and then reformatted many files)Best to review hiding whitespaces.