Skip to content

Review of 36360#36694

Merged
phil-davis merged 2 commits intomasterfrom
review-of-36360
Jan 3, 2020
Merged

Review of 36360#36694
phil-davis merged 2 commits intomasterfrom
review-of-36360

Conversation

@phil-davis
Copy link
Contributor

Description

Address code review comments from #36360

  1. rename various variables to camelCase
  2. adjust a comment to (hopefully) be clearer
  3. remove unnecessary if (isset(xxx)) checks

Related Issue

PR #36360

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

This is a code refactoring after PR #36360 so no separate changelog is needed.

Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

Looks better. @phil-davis thank you for taking care.

@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #36694 into master will decrease coverage by <.01%.
The diff coverage is 92.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36694      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
+ Complexity    19122    19118       -4     
============================================
  Files          1269     1269              
  Lines         74778    74775       -3     
  Branches       1320     1320              
============================================
- Hits          48372    48369       -3     
  Misses        26018    26018              
  Partials        388      388
Flag Coverage Δ Complexity Δ
#javascript 54.12% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.85% <92.68%> (-0.01%) 19118 <8> (-4)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Filesystem.php 69.64% <92.68%> (-0.27%) 137 <8> (-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 92759b2...8c7ef0a. Read the comment docs.

@phil-davis
Copy link
Contributor Author

Note: needs to be 'backported" to release-10.4.0 branch.

Copy link
Contributor

@sharidas sharidas left a comment

Choose a reason for hiding this comment

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

Now this looks much better. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants