Bower follow-ups #8908
Bower follow-ups #8908
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| private fun getPackageInfosWithCompleteDependencies(info: PackageInfo): Map<String, PackageInfo> { |
Check warning
Code scanning / detekt
Reports consecutive blank lines Warning
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8908 +/- ##
=========================================
Coverage 67.63% 67.63%
Complexity 1168 1168
=========================================
Files 244 244
Lines 7781 7781
Branches 867 867
=========================================
Hits 5263 5263
Misses 2161 2161
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fviernau
force-pushed
the
bower-follow-ups
branch
from
aaeff85 to
8fff913
Compare
| val alternativeNode = checkNotNull(alternativeNodes[dependencyKeyOf(info)]) | ||
| return parseDependencyTree(alternativeNode, scopeName, alternativeNodes) | ||
|
|
||
| val alternativeInfo = alternativeInfos.getValue(info.key!!) |
Check warning
Code scanning / detekt
Unsafe calls on nullable types detected. These calls will throw a NullPointerException in case the nullable value is null. Warning
| val name = pkgMeta.name.orEmpty() | ||
| val version = pkgMeta.version.orEmpty() | ||
| "$name:$version".takeUnless { name.isEmpty() || version.isEmpty() } |
There was a problem hiding this comment.
As you have with(pkgMeta) above, I believe this could be simplified to just
"$name:$version".takeUnless { name == null || version == null }
There was a problem hiding this comment.
Would this be a change in behavior in case of of them is "" ?
There was a problem hiding this comment.
Anyhow, good catch with the with(pkgMeta). This was a left over which I dropped now.
There was a problem hiding this comment.
Would this be a change in behavior in case of of them is
""?
Yes, empty names / versions would not be discarded anymore. Not sure if this would actually be any issue though.
| @@ -227,7 +227,7 @@ private fun parseDependencyTree( | |||
| // about the subtree rooted at the parent from that other node containing the full dependency | |||
| // information. | |||
| // See https://github.com/bower/bower/blob/6bc778d/lib/core/Manager.js#L557 and below. | |||
| val alternativeNode = checkNotNull(alternativeNodes[info.key]) | |||
| val alternativeNode = alternativeNodes.getValue(info.key!!) | |||
There was a problem hiding this comment.
This triggers a Detekt issue about the use of !!. I wonder whether we can work around that by making key non-nullable, e.g. by ensuring to not call it on root projects?
There was a problem hiding this comment.
Is it just me or have the warnings about !! become a bit too much.
I just wanted to make obvious what previously was a bit hidden, but this warning blocks the change right?
(If so, is it possible to go back to the previous way of flagging !! ?)
There was a problem hiding this comment.
Yes, this blocks the merge. But previously !! was not flagged at all, I believe. I prefer the current way of flagging it, and if you're really sure !! is the right thing to do in a specific case, it should be whitelisted via @Suppress("UnsafeCallOnNullableType").
There was a problem hiding this comment.
Can we revisit the decision whether we want to flag all uses of !! really?
I've spent quite some time working around cases, where !! was perfectly fine. Huge waste of time imo.
If there is a convincing rationale why !! should be forbidden, I may change my mind. But I'm not aware of any, in particular
!!seems forbidden whilecheckNotNull()seems fine- Whether
!!is ok is a case by case decision, and I'm pretty sure all approvers in our community already
pay good attention to any ocurence of!!, so flagging is not needed as we handle it already.
There was a problem hiding this comment.
I went for the supress, because the observation that the previous checkNotNull never failed implies that no lockup with a null key has been done.
There was a problem hiding this comment.
Can we revisit the decision whether we want to flag all uses of
!!really?
Sure, feel free to bring it up in a Kotlin dev meeting (not in the next three weeks, though, as both @mnonnenmacher and I will be on vacation).
fviernau
force-pushed
the
bower-follow-ups
branch
from
8fff913 to
c6289fa
Compare
Signed-off-by: Frank Viernau <[email protected]>
Signed-off-by: Frank Viernau <[email protected]>
A preceding commit refactored Bower to use data classes and a model mapper for the deserilation. So, it now operates on `PackageInfo` instances instead of on `JsonNode`s. Align variable names, function names and comments with that change, to make things consistent again. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
bower-follow-ups
branch
from
c6289fa to
fd05e38
Compare
See individual commits.