Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
============================================
- Coverage 41.16% 37.18% -3.99%
- Complexity 204 260 +56
============================================
Files 29 35 +6
Lines 838 1003 +165
============================================
+ Hits 345 373 +28
- Misses 493 630 +137
Continue to review full report at Codecov.
|
IljaN
left a comment
IljaN
left a comment
There was a problem hiding this comment.
LGTM, except a few small things. 👍
lib/Utilities/FSAccess/FSAccess.php
Outdated
| * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| */ | ||
| namespace OCA\DataExporter\Utilities\FSAccess; |
There was a problem hiding this comment.
Please move FSAccess out of Utility namespace in to the root. I dislike "Utilities" namespaces or similar because such namespace tend to be poluted with different classes where devs are too lazy to think about relationships.
There was a problem hiding this comment.
Also rename to FilesystemAccess if possible
| * Also note that concurrency isn't considered, so using this class could cause | ||
| * bugs in a concurrent environment. | ||
| */ | ||
| class FSAccess { |
There was a problem hiding this comment.
👍 For this wrapper, tried to unit-test my stream_copy changes -> very ugly
| */ | ||
| namespace OCA\DataExporter\Utilities\FSAccess; | ||
|
|
||
| class FSAccessFactory { |
There was a problem hiding this comment.
I tend to create factories only if we need different constructions, don't see why we can't construct directly, but 🆗
There was a problem hiding this comment.
I think more than 90% of the services registered in the DIContainer behave like a singleton class: although the class isn't designed as singleton, normally all the objects receive the same instance.
It's unlikely that we want the same instance in any place, so this means that we either define each service with the specific FSAccess instance to be used (increasing the boilerplate per class requiring this) or we define only one fixed instance to be used as service.
Note that the FSAccess instance is designed in a way that prevents changing the base root folder.
In addition, all the dependency wiring is defined before the commands are executed. The problem is that the target path where the exported info resides is known during the command execution. We can't place the jail during the dependency wiring because at that point is unknown.
|
This PR has been approved already but is falling behind the quickly changing master branch. |
|
@jvillafanez Maybe we need to split this PR into smaller parts. |
|
From my point of view, this should have been merged before starting working on things because it contains a refactor that touches and modifies a lot of things. At this point, after having been merged things, it's too late to do anything with this branch. |
4 participants
Summing up the list of changes:
FSAccessclass to handle FS access instead of symfony's filesystem. For the extra features added for now:Known issues:
Pending things on this PR:
Additional things pending to do later (they won't be done in this PR):
Closes #28