Skip to content
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

Introduce app info xml parser including basic unit test - necessary for ... #12393

Merged
merged 3 commits into from
Nov 25, 2014

Conversation

DeepDiver1975
Copy link
Member

...#10777

@ghost
Copy link

ghost commented Nov 24, 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/3145/
🚀 Test PASSed. 🚀

@ghost
Copy link

ghost commented Nov 24, 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/3153/
🚀 Test PASSed. 🚀

@DeepDiver1975
Copy link
Member Author

@PVince81 @LukasReschke @schiesbn @blizzz please review - thx

*/
public function parse($file) {
if (!file_exists($file)) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Why not throwing an exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

keep the current behavior

@LukasReschke
Copy link
Member

Besides the comments: Great stuff 👍

@blizzz
Copy link
Contributor

blizzz commented Nov 24, 2014

Tested is only the successful case, but what if the XML file is broken or invalid? There should be defined way of handling this, too.

@DeepDiver1975
Copy link
Member Author

Tested is only the successful case, but what if the XML file is broken or invalid? There should be defined way of handling this, too.

will add such tests asap - thx

@DeepDiver1975
Copy link
Member Author

@LukasReschke @blizzz thanks for the review - comments have been addressed

@blizzz
Copy link
Contributor

blizzz commented Nov 25, 2014

code looks, apps page is still working 👍

@ghost
Copy link

ghost commented Nov 25, 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/3205/
🚀 Test PASSed. 🚀

@ghost
Copy link

ghost commented Nov 25, 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/3207/
🚀 Test PASSed. 🚀


/**
* @param string $file
* @return null|array
Copy link
Member

Choose a reason for hiding this comment

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

Document that "null" should be considered an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

well - I expect changes to the interface in the next iterations anyhow - will take care if it ...

@LukasReschke
Copy link
Member

👍

@DeepDiver1975
Copy link
Member Author

I'm missed the entity-loader thingy - just a sec

@DeepDiver1975
Copy link
Member Author

will merge after jenkins success

@scrutinizer-notifier
Copy link

The inspection completed: 9 updated code elements

@ghost
Copy link

ghost commented Nov 25, 2014

💣 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/3217/

Build result: FAILURE

[...truncated 17 lines...] > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/12393/merge^{commit} # timeout=10Checking out Revision ea5889f849b393a0587a19bc9b3e29e327062065 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f ea5889f849b393a0587a19bc9b3e29e327062065 > git rev-list f3818a6b8eb28f1b42a7da6c75b12c6f71c216c3 # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveCleaning workspace > git rev-parse --verify HEAD # timeout=10Resetting working tree > git reset --hard # timeout=10 > git clean -fdx # timeout=10 > git submodule foreach --recursive git reset --hard # timeout=10 > git submodule foreach --recursive git clean -fdx # timeout=10Triggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 3 second
💣 Test FAILed. 💣

@DeepDiver1975 DeepDiver1975 deleted the app-info-xml-parser branch November 25, 2014 14:57
@DeepDiver1975 DeepDiver1975 merged commit 5ce34fb into master Nov 25, 2014
@PVince81
Copy link
Contributor

Looks similar to #9803

@bartv2
Copy link
Contributor

bartv2 commented Dec 3, 2014

To bad this needs to be redeveloped after waiting on the other PR :-(

@DeepDiver1975
Copy link
Member Author

To bad this needs to be redeveloped after waiting on the other PR :-(

well - shit happens - but to be honest: it's within the responsibility of every dev to get his changes merged - if stuff is lost on the road ....

No offense - happens to my PRs as well 🙊

@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.

6 participants