Missing @file blocks, wrong styling at dot operator, misuse of db_query in hook_install() and a serious one: feedapi_install() ereased some variables, which is foolness, it should be done in _uninstall(), i assume.

CommentFileSizeAuthor
#5 499730-3_code_style.patch8.48 KBaron novak
#4 499730-2_code_style.patch8.49 KBaron novak
#1 499730-1_code_style.patch8.65 KBalex_b
feedapi_code_style.patch8.79 KBaron novak

Comments

alex_b’s picture

alex_b commented
StatusFileSize
new 499730-1_code_style.patch8.65 KB

Good cleanup. I fixed some minor issues:

- Use imparative instead of third person for comments - i. e. "Create new content type." instead of "Creates new content type."
- @file comments should be more generic in install files - I limited them all to a very generic "Install file for..." which I feel is in line with the comments for hook implementations.
- Cleaned up some language.
- Simplified drush @file comments.

Open question: there are three occasions where regexes have been changed? Was this on purpose? E. g.: 

-      preg_match_all('/type\s*=\s*("|'. "'" .')([A-Za-z\/+]*)("|'. "'" .')/si', $link, $mime);
+      preg_match_all('/type\s*=\s*("|'."'".')([A-Za-z\/+]*)("|'."'".')/si', $link, $mime);
aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

#1: The content of the regexes have not been changed, it's code style question.
"string concatenation should be formatted without a space separating the operators (dot .) and a quote" - coder module
According to this: "." - this is the good :)

However this is changed in these days: http://drupal.org/coding-standards
"Always use a space between the dot and the concatenated parts to improve readability." - as I know this is for D7

However this is a really marginal question.

alex_b’s picture

alex_b commented

#2 : stupid me :) haven't read the string properly, didn't notice that these " where actually delimiting the very regex string. But it's weird how this is being used to excape quotes. Shouldn't we use the proper escaple character for this?

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 499730-2_code_style.patch8.49 KB

Rerolled.
In fact those different style quotes were really amigous :)

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 499730-3_code_style.patch8.48 KB

One of the regular expressions was skipped from the update.

alex_b’s picture

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

Ready to be committed.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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