The Media repo has been a hybrid monster of old and new code for too long. We should pick a standard and go with it. Here's what I'm proposing:

All .inc files go in includes/
javascript file is renamed "js"
theme is renamed "templates"

All files are named in the format first.second.word.extension (I personally think this is a bad naming syntax because it does not distinguish between spaces in the name and the extension, but it seems to be quite popular w/ Drupal folks these days... If people would rather see first_second_word.extension, I'm cool with that too.

CommentFileSizeAuthor
#2 0001-Issue-1106922-Change-filenames-and-directory-structu.patch381.97 KBJacobSingh

Comments

arthurf’s picture

arthurf commented

All .inc files go in includes/
javascript file is renamed "js"
theme is renamed "templates"

Makes sense to me.

As per the file naming conventions I'd like to see the javascript files loose the '-' in their file names. I'm not sure where I weigh in on the use of . in file names, but as long as it's consistent it seems less bad to me.

JacobSingh’s picture

JacobSingh commented
Status: Active » Needs review
StatusFileSize
new 0001-Issue-1106922-Change-filenames-and-directory-structu.patch381.97 KB

This is a biggie :)

Moves all .inc files to "includes"
Renames all files to use dots.between.parts except where it is a.compound_term.extension
Moves classes to includes
renames javascript to js
renames theme to templates

Status: Needs review » Needs work

The last submitted patch, 0001-Issue-1106922-Change-filenames-and-directory-structu.patch, failed testing.

JacobSingh’s picture

JacobSingh commented

Testbot doesn't like my patch made with git format-patch. I'll research this, but please review it anyway.

effulgentsia’s picture

effulgentsia commented

Sorry. I can't apply the patch, or make sense of it manually. Can you re-roll? Or create a git fork with reviewable commits?

> git apply 0001-Issue-1106922-Change-filenames-and-directory-structu.patch
0001-Issue-1106922-Change-filenames-and-directory-structu.patch:883: trailing whitespace.

0001-Issue-1106922-Change-filenames-and-directory-structu.patch:898: trailing whitespace.

0001-Issue-1106922-Change-filenames-and-directory-structu.patch:1212: trailing whitespace.

0001-Issue-1106922-Change-filenames-and-directory-structu.patch:1541: trailing whitespace.

0001-Issue-1106922-Change-filenames-and-directory-structu.patch:1545: trailing whitespace.

error: patch failed: README.txt:5
error: README.txt: patch does not apply
error: patch failed: javascript/wysiwyg-media.js:1
error: javascript/wysiwyg-media.js: patch does not apply
error: patch failed: media.admin.inc:1
error: media.admin.inc: patch does not apply
error: patch failed: media.browser.inc:1
error: media.browser.inc: patch does not apply

effulgentsia’s picture

effulgentsia commented
Assigned: Unassigned » effulgentsia

Oops. It's cause I tried applying on master instead of 7.x-1.x. it does apply successfully on 7.x-1.x. But the files lose history that way. I'm attempting to do this in individual logical commits to the "1106922-filelayout" branch. Will post again here when that's all pushed and ready for review before merging back to 7.x-1.x.

JacobSingh’s picture

JacobSingh commented

Ahh... I already have individual commits in another branch. I rebased this so that it would be in one patch without all those commits :(

Anyway, I guess w/o some git magic which I don't know the incantations of it would be impossible to un-rebase. The change is totally described in the comments. It really just does those 4-5 things.

I'll see if I can get them back.
Best,
Jacob

JacobSingh’s picture

JacobSingh commented

Anyway, my commits aren't particularly clean (as is the git way) and I didn't feel like rebasing again since you had already started, but if it is helpful, I pushed the branch up:

1106922_filemove

effulgentsia’s picture

effulgentsia commented
Assigned: effulgentsia » Unassigned
Status: Needs work » Reviewed & tested by the community

#2 is pushed to the "1106922-filelayout" branch in a way that retains history. Go ahead and merge to 7.x-1.x when ready. Note that this will break modules that load media include files. For example, Media Gallery will need to update its media_gallery_add_images_link_pre_render() function. So when you merge this to 7.x-1.x, please announce it in the channels you think is appropriate.

I tested this in the context of Drupal Gardens (so, including wysiwyg and media_youtube), and Media Gallery seemed to be the only place that had obvious breakage as a result of this.

JacobSingh’s picture

JacobSingh commented
Status: Reviewed & tested by the community » Fixed

Aw snap... git fail... I pushed 7.x-1.x accidentally from my old changes.

So there are a bunch of garbage commits now in 7.x-1.x. Sorry, still learning. Marking this fixed since it is in.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.