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

The class name is Movie NOT Movies #12313

Merged
merged 1 commit into from
Nov 25, 2014
Merged

The class name is Movie NOT Movies #12313

merged 1 commit into from
Nov 25, 2014

Conversation

oparoz
Copy link
Contributor

@oparoz oparoz commented Nov 20, 2014

No description provided.

@ghost
Copy link

ghost commented Nov 20, 2014

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@oparoz
Copy link
Contributor Author

oparoz commented Nov 20, 2014

This contribution is MIT licensed

@nickvergessen
Copy link
Contributor

The problem here is even worse, the class name does not match the file name.
I think we should fix the class name instead.

I checked the code and the class name only appears in the class file, so change it there.

@MorrisJobke
Copy link
Contributor

@nickvergessen We can also fix the file name

@nickvergessen
Copy link
Contributor

yeah would also make sense since all others are singulars... so dont care too much, either fix the class name, or fix the docs and the file name

@MorrisJobke
Copy link
Contributor

I would vote for the rename of the file. @oparoz Can you add the rename of the file into this PR? Thanks

@LukasReschke
Copy link
Member

@owncloud-bot This is okay to test.

@oparoz
Copy link
Contributor Author

oparoz commented Nov 20, 2014

That should be it. Let me know if I've forgotten something...

@nickvergessen
Copy link
Contributor

Mind squashing all commits together?
If you don't know what squashing means, either jump on irc: freenode #owncloud-dev or say it here, then someone else can try to do it for you.

@MorrisJobke
Copy link
Contributor

👍

@nickvergessen
Copy link
Contributor

Now you messed up the file rename and duplicated the file instead.

@oparoz
Copy link
Contributor Author

oparoz commented Nov 20, 2014

@nickvergessen - Well, that's the way "Github for Windows" does it. The change said "renamed", but then a new file is created during the git push --force and the old one is left behind and has to be deleted in another commit.

Updating 284ba5b..cc26d50
Fast-forward
 config/config.sample.php                      | 2 +-
 lib/private/preview.php                       | 4 ++--
 lib/private/preview/{movies.php => movie.php} | 0
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename lib/private/preview/{movies.php => movie.php} (100%)

@ghost
Copy link

ghost commented Nov 21, 2014

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@MorrisJobke
Copy link
Contributor

@owncloud-bot this is okay to test

@nickvergessen
Copy link
Contributor

@MorrisJobke this needs to be squashed again, otherwise the history of movie.php is lost, which is easily avoidable.

@oparoz
Copy link
Contributor Author

oparoz commented Nov 21, 2014

After an upgrade, GFW was able to do it all in one step \o/

@nickvergessen
Copy link
Contributor

Can't really test this on windows, because movie is disabled.
But from the code it looks okay: 👍

@MorrisJobke
Copy link
Contributor

@owncloud-bot this is okay to test

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 Jenkins refuses to test this 😠

@nickvergessen
Copy link
Contributor

@MorrisJobke that is because it cant be merged.
Needs a rebase

@MorrisJobke
Copy link
Contributor

This wasn't the case when I wrote this

@oparoz
Copy link
Contributor Author

oparoz commented Nov 24, 2014

Could be because of other patches against the Movie class which gets renamed in this one

@MorrisJobke
Copy link
Contributor

@oparoz Can I ask you to rebase this on current master and fix the merge conflict?

@oparoz
Copy link
Contributor Author

oparoz commented Nov 25, 2014

@MorrisJobke - Done

@MorrisJobke
Copy link
Contributor

@owncloud-bot retest this please

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@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/3215/
🚀 Test PASSed. 🚀

DeepDiver1975 added a commit that referenced this pull request Nov 25, 2014
The class name is Movie NOT Movies
@DeepDiver1975 DeepDiver1975 merged commit 5531882 into owncloud:master Nov 25, 2014
@oparoz oparoz deleted the patch-2 branch November 25, 2014 13:09
@ghost
Copy link

ghost commented Feb 21, 2015

Hi,

backporting this to OC7 could fix #14316

@oparoz
Copy link
Contributor Author

oparoz commented Feb 21, 2015

@MorrisJobke - Backport?

@karlitschek
Copy link
Contributor

@MorrisJobke Can you backport?

@nickvergessen
Copy link
Contributor

Will take care of it later

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 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.

7 participants