Skip to content

Migrate new app settings to AppFramework#11600

Merged
LukasReschke merged 1 commit intomasterfrom
refactor-appsettings-to-app-framework
Oct 28, 2014
Merged

Migrate new app settings to AppFramework#11600
LukasReschke merged 1 commit intomasterfrom
refactor-appsettings-to-app-framework

Conversation

@LukasReschke
Copy link
Copy Markdown
Member

Let's migrate those two new files. - Code logic is not touched, this is only for the sake of preventing more ugly ajax/* files having added.

@DeepDiver1975 As discussed.

Let's migrate those two new files.
@LukasReschke
Copy link
Copy Markdown
Member Author

(Writing sane unit tests for this seems like a nearly impossible task considered the fact that OC_App is completely static)

@scrutinizer-notifier
Copy link
Copy Markdown

The inspection completed: 10 new issues, 6 updated code elements

@ghost
Copy link
Copy Markdown

ghost commented Oct 15, 2014

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

@icewind1991
Copy link
Copy Markdown
Contributor

👍 tested

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your tabbing is going too far!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mhm - works for me locally, according to our coding guidelines:

Use tabs to indent
A tab is 4 spaces wide

And then it matches:

screen shot 2014-10-28 at 11 12 31

@PVince81
Copy link
Copy Markdown
Contributor

Code looks good 👍

How about adding unit tests now that this is nicely encapsulated ? 😈

@PVince81
Copy link
Copy Markdown
Contributor

Hmmm, seems that \OC_App::listAllApps(true); isn't... Can OC_App be injected as well ?
Then unit tests could be written that mock it.

@LukasReschke
Copy link
Copy Markdown
Member Author

Hmmm, seems that \OC_App::listAllApps(true); isn't... Can OC_App be injected as well ?
Then unit tests could be written that mock it.

That would require some refactoring on OC_App as far I can see, let's schedule this for later.

LukasReschke added a commit that referenced this pull request Oct 28, 2014
…ramework

Migrate new app settings to AppFramework
@LukasReschke LukasReschke merged commit 437a660 into master Oct 28, 2014
@LukasReschke LukasReschke deleted the refactor-appsettings-to-app-framework branch October 28, 2014 10:13
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants