Move more logic from addMountPoint into separate functions #8583

RobinMcCorkell · 2014-05-13T17:56:33Z

I will get Scrutinizer to give something higher than an F for addMountPoint... also this reduces duplicated code and (IMO) makes the logic a bit clearer

ghost · 2014-05-13T18:53:08Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4769/

LukasReschke · 2014-05-14T19:38:30Z

Create some unit tests and you will get my +1 ;-)

RobinMcCorkell · 2014-05-14T20:13:13Z

@LukasReschke The new functions are private, and are used in existing functions (which have unit tests already), so how would that work?

LukasReschke · 2014-05-14T20:19:16Z

http://sebastian-bergmann.de/archives/881-Testing-Your-Privates.html

LukasReschke · 2014-05-14T20:24:50Z

Btw. I can't find an unittest for removeMountPoint. If we have unittests for all public functions then testing the private ones is not relevant anymore.

RobinMcCorkell · 2014-05-14T22:01:08Z

@LukasReschke Done 😄

ghost · 2014-05-14T22:58:25Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4794/

bantu · 2014-05-15T20:11:34Z

apps/files_external/lib/config.php

+			if (is_null($user)) {
+				return true;
+			} else {
+				if (isset($personalBackends[$backend])) {


Merge into "else if"

ghost · 2014-05-16T09:35:43Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4825/

PVince81 · 2014-05-21T10:16:10Z

@Xenopathic before refactoring, please make sure that we're fixing an actual issue and that there is a direct need for it.
The reason is that there is always a risk of breaking existing functionality and it requires taking time for regression testing. But if there is another bugfix or something that will need regression testing anyway, it would also cover the refactoring.
Best practice is to only refactor when it fixes an actual issue, for which the tests will cover the refactoring as well, as to save testing time.

The code looks good but haven't had time to test it yet.

PVince81 · 2014-05-21T10:17:29Z

Still, kudos for making the code more readable 😄

PVince81 · 2014-05-22T12:38:41Z

Please rebase

RobinMcCorkell · 2014-05-22T13:51:34Z

I split the new priority code into a separate function - see getBackendDefaultPriority()

ghost · 2014-05-22T15:56:52Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4947/

ghost · 2014-05-22T19:48:59Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4954/

PVince81 · 2014-06-06T07:36:10Z

I didn't have time to test this yet, sorry.

@DeepDiver1975 are you ok with this PR ?

PVince81 · 2014-06-06T09:01:46Z

@LukasReschke the unit tests are here, can you have a another look ?

PVince81 · 2014-06-06T09:06:18Z

I had a quick test and adding mount points still work.
Also, the merging of configs when updating them work as well.

👍

Needs a second reviewer @LukasReschke @icewind1991

PVince81 · 2014-06-06T13:05:23Z

@Xenopathic please rebase

ghost · 2014-06-06T22:46:13Z

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5466/

scrutinizer-notifier · 2014-06-17T12:57:51Z

A new inspection was created.

RobinMcCorkell · 2014-06-17T12:59:53Z

Is this something that we want merged then? As you say @PVince81, it does make the code significantly more readable, but it also paves the way for another PR I have planned (when I get the next version my other project released). In that case, this needs a second reviewer. @LukasReschke you happy with this now?

PVince81 · 2014-06-17T13:01:45Z

Trouble is we are already past feature freeze, so it might need to wait for OC 8 to be merged, sorry :-/

ghost · 2014-06-17T14:04:03Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5644/

RobinMcCorkell · 2014-06-17T15:02:27Z

OK, in that case this can wait: it doesn't significantly affect performance

PVince81 · 2014-07-21T11:21:44Z

@bantu you'll probably enjoy reviewing this refactoring ?

ghost · 2014-10-06T08:12:43Z

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng/80/

ghost · 2014-10-15T09:19:58Z

💣 Test FAILed. 💣
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/152/
💣 Test FAILed. 💣

PVince81 · 2014-10-31T12:22:33Z

@Xenopathic do we still want this ?
I'd vote to rather work on improving the whole thing radically as discussed in #11261

RobinMcCorkell · 2014-10-31T12:35:50Z

@PVince81 Yeah, let's kill it. I'll leave the branch though, in case someone can benefit from the changed code

MorrisJobke · 2015-01-06T16:15:14Z

@Xenopathic You can always restore the branch here on github, if you want ;)

RobinMcCorkell · 2015-01-06T21:46:41Z

@MorrisJobke For the time being, at least until GitHub does garbage collection :)

RobinMcCorkell mentioned this pull request May 14, 2014

Priority on external storage mounts #8163

Merged

RobinMcCorkell mentioned this pull request May 15, 2014

Dynamically update stored credentials in mount config #8167

Closed

3 tasks

bantu reviewed May 15, 2014
View reviewed changes

apps/files_external/lib/config.php

if (is_null($user)) {

return true;

} else {

if (isset($personalBackends[$backend])) {

Copy link

bantu May 15, 2014

Choose a reason for hiding this comment

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

Merge into "else if"

MorrisJobke added the 4 - Developing label May 21, 2014

Robin McCorkell added 3 commits June 17, 2014 13:57

Move more logic from addMountPoint into separate functions

a4c09d1

Add removeMountPoint unit tests

ce07c51

Optimize isValidBackend

fa35b7d

scolebrook force-pushed the master branch from 4a4ea1d to 9dea79e Compare October 10, 2014 13:32

RobinMcCorkell closed this Oct 31, 2014

MorrisJobke deleted the better-addmountpoint branch January 6, 2015 16:14

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move more logic from addMountPoint into separate functions #8583

Move more logic from addMountPoint into separate functions #8583

RobinMcCorkell commented May 13, 2014

ghost commented May 13, 2014

LukasReschke commented May 14, 2014

RobinMcCorkell commented May 14, 2014

LukasReschke commented May 14, 2014

LukasReschke commented May 14, 2014

RobinMcCorkell commented May 14, 2014

ghost commented May 14, 2014

bantu May 15, 2014

ghost commented May 16, 2014

PVince81 commented May 21, 2014

PVince81 commented May 21, 2014

PVince81 commented May 22, 2014

RobinMcCorkell commented May 22, 2014

ghost commented May 22, 2014

ghost commented May 22, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

ghost commented Jun 6, 2014

scrutinizer-notifier commented Jun 17, 2014

RobinMcCorkell commented Jun 17, 2014

PVince81 commented Jun 17, 2014

ghost commented Jun 17, 2014

RobinMcCorkell commented Jun 17, 2014

PVince81 commented Jul 21, 2014

ghost commented Oct 6, 2014

ghost commented Oct 15, 2014

PVince81 commented Oct 31, 2014

RobinMcCorkell commented Oct 31, 2014

MorrisJobke commented Jan 6, 2015

RobinMcCorkell commented Jan 6, 2015

Move more logic from addMountPoint into separate functions #8583

Move more logic from addMountPoint into separate functions #8583

Conversation

RobinMcCorkell commented May 13, 2014

ghost commented May 13, 2014

LukasReschke commented May 14, 2014

RobinMcCorkell commented May 14, 2014

LukasReschke commented May 14, 2014

LukasReschke commented May 14, 2014

RobinMcCorkell commented May 14, 2014

ghost commented May 14, 2014

bantu May 15, 2014

Choose a reason for hiding this comment

ghost commented May 16, 2014

PVince81 commented May 21, 2014

PVince81 commented May 21, 2014

PVince81 commented May 22, 2014

RobinMcCorkell commented May 22, 2014

ghost commented May 22, 2014

ghost commented May 22, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

PVince81 commented Jun 6, 2014

ghost commented Jun 6, 2014

scrutinizer-notifier commented Jun 17, 2014

RobinMcCorkell commented Jun 17, 2014

PVince81 commented Jun 17, 2014

ghost commented Jun 17, 2014

RobinMcCorkell commented Jun 17, 2014

PVince81 commented Jul 21, 2014

ghost commented Oct 6, 2014

ghost commented Oct 15, 2014

PVince81 commented Oct 31, 2014

RobinMcCorkell commented Oct 31, 2014

MorrisJobke commented Jan 6, 2015

RobinMcCorkell commented Jan 6, 2015