-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use FileUtils instead of function in Scanner #32375
Conversation
lib/public/Files/Cache/IScanner.php
Outdated
@@ -69,13 +77,7 @@ public function scan($path, $recursive = self::SCAN_RECURSIVE, $reuse = -1, $loc | |||
* @param string $file | |||
* @return boolean | |||
* @since 9.0.0 | |||
* @deprecated use FileUtils | |||
*/ | |||
public static function isPartialFile($file); |
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 dont even think if anybody is using this interface.. why would it? It was circumvented anyways in View and used Cache/Scanner directly
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.
Can we delete it?
Codecov Report
@@ Coverage Diff @@
## master #32375 +/- ##
============================================
- Coverage 64.03% 64.03% -0.01%
+ Complexity 18579 18578 -1
============================================
Files 1171 1172 +1
Lines 69876 69874 -2
Branches 1267 1267
============================================
- Hits 44747 44743 -4
- Misses 24759 24761 +2
Partials 370 370
Continue to review full report at Codecov.
|
lib/public/Files/Cache/IScanner.php
Outdated
@@ -69,13 +76,7 @@ public function scan($path, $recursive = self::SCAN_RECURSIVE, $reuse = -1, $loc | |||
* @param string $file | |||
* @return boolean | |||
* @since 9.0.0 | |||
* @deprecated use FileUtils |
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.
FileUtils is OC namespace - it cannot be used to deprecate an OCP method.
keep it here
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.
@DeepDiver1975 can we remove it at all? I don't think anybody uses this interface, since why would he?
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.
you can deprecate it and we can remove the api call in some month/years
@@ -0,0 +1,41 @@ | |||
<?php | |||
/** | |||
* Copyright (c) 2012 Robin Appelman <icewind@owncloud.com> |
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.
wrong copytight - please adjust
* | ||
* @package Test\Files\Utils | ||
*/ | ||
class FileUtilsSTest extends \Test\TestCase { |
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.
-S
* | ||
* @package Test\Files\Utils | ||
*/ | ||
class FileUtilsSTest extends \Test\TestCase { |
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.
import TestCase
d9402c9
to
1a34d83
Compare
@DeepDiver1975 all tests passing, review addressed |
conflicting ... please resolve |
1a34d83
to
266ea5b
Compare
@DeepDiver1975 rebased |
if (\pathinfo($path, PATHINFO_EXTENSION) === 'part') { | ||
return true; | ||
} | ||
if (\strpos($path, '.part/') !== false) { |
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.
in some cases you replaced this part was missing, I wonder if this will break some undiscovered cases with "part folders"
let's keep this PR master only just in case
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.
👍
master only, see #32375 (comment) |
Is #32375 (comment) still correct? |
let's play it safe => no backport. |
As in the topic - addressing technical dept