Repository navigation
Chore: Code hygiene - warn-numeric-widen - #2588
andy-hf-kwok wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2588 +/- ##
============================================
+ Coverage 56.12% 58.45% +2.33%
- Complexity 976 1456 +480
============================================
Files 119 159 +40
Lines 11743 13892 +2149
Branches 2251 2375 +124
============================================
+ Hits 6591 8121 +1530
- Misses 4012 4570 +558
- Partials 1140 1201 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| record.add(0, i % 2 == 0) | ||
| record.add(1, i.toByte) | ||
| record.add(2, i.toShort) | ||
| record.add(1, i.toByte.toInt) |
There was a problem hiding this comment.
its probably can be directly casted to int?
There was a problem hiding this comment.
I believe overflow behaviour will be different and lead to unexpected and incorrect results
There was a problem hiding this comment.
Actually I think if the test suite is mean to examine the behaviour when Byte or Short being passed, then we should keep it as it is.
I have reverted some .toInt on test suites for this purpose and add a suppression annotation instead.
@comphead @EmilyMatt
| record.add(0, i % 2 == 0) | ||
| record.add(1, i.toByte) | ||
| record.add(2, i.toShort) | ||
| record.add(1, i.toByte.toInt) |
There was a problem hiding this comment.
same, can be probably converted directly
There was a problem hiding this comment.
see above reply, I believe in this case it's more appropriate to keep it as it is, so that we can assert the implicit conversation behaviour on test suites.
a1f1b3c to
c1b4b86
Compare
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
c047bd0 to
8712bde
Compare
|
@comphead @mbutrovich I have removed some explicit conversions and added a suppression instead, as it seems more appropriate. |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
This PR aims to address the
-Wnumeric-widenwarning in the codebase.As the overall scope of strict-warning compliance is quite large, this change focuses on resolving this specific warning type as a first step. Future PRs will continue addressing other warning incrementally.
Partially closes #. #2255
Rationale for this change
To clean up the code as per static check with the hope to resolve all warning and promote the restrict warning profile for the default CI build to uphold Scala code quality.
What changes are included in this PR?
To perform explicit type conversion instead of relying on runtime implicit cast.
How are these changes tested?
-Wnumeric-widenoption on.