Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #1015: Enable an detekt rule for wrap blocks in { } #1040

Conversation

vadimsemenov
Copy link
Contributor

@vadimsemenov vadimsemenov commented Oct 14, 2018

Closes #1015

@vadimsemenov vadimsemenov requested a review from a team as a code owner October 14, 2018 18:12
@codecov
Copy link

codecov bot commented Oct 14, 2018

Codecov Report

Merging #1040 into master will increase coverage by 0.04%.
The diff coverage is 62.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1040      +/-   ##
============================================
+ Coverage     79.67%   79.72%   +0.04%     
+ Complexity     1491     1488       -3     
============================================
  Files           194      194              
  Lines          5807     5805       -2     
  Branches        936      934       -2     
============================================
+ Hits           4627     4628       +1     
+ Misses          742      740       -2     
+ Partials        438      437       -1
Impacted Files Coverage Δ Complexity Δ
...illa/components/ui/progress/AnimatedProgressBar.kt 53.16% <0%> (+0.07%) 14 <0> (ø) ⬇️
...mponents/browser/engine/system/SystemEngineView.kt 82.65% <0%> (ø) 29 <0> (ø) ⬇️
...ervice/fretboard/source/kinto/SignatureVerifier.kt 78.02% <0%> (ø) 21 <0> (ø) ⬇️
.../browser/engine/system/matcher/ReversibleString.kt 81.48% <100%> (ø) 9 <2> (ø) ⬇️
...zilla/components/browser/session/SessionManager.kt 89.13% <100%> (ø) 73 <0> (ø) ⬇️
...mponents/browser/toolbar/display/DisplayToolbar.kt 89.1% <100%> (+1.28%) 34 <0> (-3) ⬇️
...tboard/source/kinto/HttpURLConnectionHttpClient.kt 88.88% <100%> (ø) 7 <0> (ø) ⬇️
...ents/ui/autocomplete/InlineAutocompleteEditText.kt 73.23% <50%> (ø) 42 <0> (ø) ⬇️
...ponents/browser/engine/system/matcher/WhiteList.kt 86.53% <60%> (ø) 19 <4> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b94c984...2d945b9. Read the comment docs.

Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This looks great. I wonder where the asSequence() changes are coming from?

@@ -312,6 +310,7 @@ internal class DisplayToolbar(
// +-------------+------------------------------------------------+

val navigationActionsWidth = navigationActions
.asSequence()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two asSequence() changes seem unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed it because of IDEA inspection.

If we apply call chain on List, on each call new ArrayList will be created. So in worst case O((number of calls in chain) * (size of initial list)) aux memory will be allocated. asSequence fixes it and creates only (more on less) a new Sequence on each call.
It should improve both memory consumption and performance.

In case it seems unrelated, I can extract it to another pull request.
If you'd like me to do this, should I also extract fixes in documentation? As It seems equally unrelated to the original issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pocmo I've run that inspection on the whole project and found another warning here:

return experiments.filter { isInExperiment(context, ExperimentDescriptor(it.name)) }.toList()

So it may worth to extract it to separate issue and pull request. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pocmo, @Amejia481 any updates on this?

@pocmo pocmo merged commit 5c56606 into mozilla-mobile:master Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants