@see should be one line, no full stop
No trailing whitespace
No tabs
Non-code lines should wrap at 80 characters
$variables is preferred over $vars
Space between if/foreach and (
Comments should end in full stop
Comments should begin with a capital letter
No empty line between common doxygen elements (@param, @see)
With @param, (optional) should go on the next line
screenshot = screenshot.png is the default in .info
version should not be included in .info, will be added by packaging script
LICENSE.txt should not be included, will be added by packaging script

Patch forthcoming.

CommentFileSizeAuthor
#5 omega-coding-standards-eol-eof-1277886.patch185.68 KBjwilson3
#4 omega-1277886-4.patch198.63 KBtim.plunkett
#1 omega-1277886-1.patch199.63 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new omega-1277886-1.patch199.63 KB

See attached. Can be committed with curl http://drupal.org/files/issues/omega-1277886-1.patch | git am.

cellar door’s picture

cellar door commented
Assigned: Unassigned » himerus

Can we include in 3.1?

himerus’s picture

himerus commented
Status: Needs review » Needs work

Patch fails to apply cleanly to current 7.x-3.x version (presumably from some recent commits)

Will add to the 3.2 release if we can get the patch cleaned up in time.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Minor » Normal
Status: Needs work » Needs review
StatusFileSize
new omega-1277886-4.patch198.63 KB

I did as much as I could with regular expressions and such. Something is wrong with the text editor used to write these files, since every one of them was missing a EOF after the EOL. In textmate, this means you were missing the blank line at the end of the file. (Editors like vim wouldn't show it as a blank line.

Also, there were hundreds of trailing whitespace.

@see needs to have the url or function directly after it on that line, with no punctuation.
Multiple @param lines shouldn't have blank lines between them
But whenever you switch the type of @, you should. I.e., a blank line between a set of @param and the @return.

Function comments should be a single line, I had to add some todos.

See http://drupal.org/node/1354 for details.

The hosted PAReview tool is helpful, consider checking that after committing this as it might need a followup: http://ventral.org/pareview/httpgitdrupalorgprojectomegagit-7x-3x

There are just too many whitespace warnings to make the output useful.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new omega-coding-standards-eol-eof-1277886.patch185.68 KB

Damn, I just created another issue for this, that i just marked as a duplicate #1449698: Improve Omega's coding standards (eol, eof, and column wrap).

Sorry for not searching first. :-/

In that issue though, I've provided fixes for just two issues (eol whitespace, and eof newline).

Perhaps we could separate each fix into a different patch to make it easier to check this, and commit them progressively?

Since the EOL and EOF are intertwined, and actually fixing some of the EOFs does fix the eol, in some cases... I've combined these two into a single patch... attached here.

Copying the cleanup commands from that issue:

# Clean EOF On Mac
find . -type f -not -path "*.git*" -not -name "*.png" -not -name "*.gif" -print0 | xargs -0 sed -i '' '/^$/N;/\n$/D' 

# Clean EOL on Mac: (sed -i requires an extra set of empty quotes)
find . -type f -not -path "*.git*" -not -name "*.png" -not -name "*.gif" -print0 | xargs -0 sed -i '' 's/ *$//g'

Status: Needs review » Needs work

The last submitted patch, omega-coding-standards-eol-eof-1277886.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review

Not sure what #5 broke, but #4 is ready to go. Perhaps that can be committed first and the other rerolled? Since mine is 185KB and applies cleanly

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

#facepalm, I didnt format the patch correctly.

Patch in #4 does apply cleanly. And running my cleanup commands from #5 after having applied #4 only fixes a couple instances of \n\n\n (replaces them with \n\n), so it looks like you got all of the eol and eof errors!

Good work.

Lets get this in ASAP while it still applies!!!

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
Status: Needs review » Reviewed & tested by the community

To clarify, patch in #4 is RTBC.

himerus’s picture

himerus commented
Status: Needs review » Reviewed & tested by the community

I will test the patch in #4 momentarily in my dev environment and see how it goes... I'm unsure since I just turned on the automated patch testing in the project settings how this all works!!!

Thanks for the patch(es)!!

himerus’s picture

himerus commented
Assigned: himerus » Unassigned
Status: Reviewed & tested by the community » Fixed

This has been committed to 7.x-3.x and will be available in the 3.2 release.

I have a feeling another cleanup will likely be needed at/or prior to the 3.2 release with a lot of upcoming changes.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

Great! Thanks Jake.

Status: Fixed » Closed (fixed)

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