patch based on http://drupal.org/node/164946

CommentFileSizeAuthor
#18 code_style_node_module_18.patch13.2 KBpancho
#13 code_style_node_module_13.patch10.58 KBpancho
#11 code_style_node_module_3.patch10.58 KBpancho
#10 code_style_node_module_2.patch10.16 KBpasqualle
#5 code_style_node_module_1.patch12.15 KBpasqualle
code_style_node_module.patch12.18 KBpasqualle

Comments

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Active » Needs review
pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Category: task » bug
Status: Needs review » Active

See my comment on the other issue

Should be categorized as code style bug - these days tasks are not looked at enough...

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Active » Needs review
webernet’s picture

webernet commented
Status: Needs review » Needs work

Comments should always be full sentences and end with a period.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new code_style_node_module_1.patch12.15 KB

fixed

please give me a phpdoc for function node_admin_nodes, because it is missing

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

This looks good except for one thing:
I wouldn't remove linefeeds in lines that are already very long, as you did in node_configure_rebuild_confirm(). Rather would I add some more linefeeds here.

I'm also not sure about the changed doxygen comment:

- *   A node array or node object.
+ *   A node object.

Looks like node_view() can indeed take a node array as argument.

Otherwise this looks good!

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

the changes were in patch #164946 written by dmitrig01, I just tried to roll everything in it

I don't know if node_view() accept array or not. should the comment remain as it is originally?

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

node_view() converts $node to an object, if it is an array, so it obviously does take a node array. I'd just keep that.

The missing phpdoc for node_admin_nodes() is not nice but not the only one missing.
But I would say, we don't have to make everything perfect at once - correcting some 50% of the code style issues is good to begin with. Next time we improve the next bit, it's a neverending story anyway. :)

Thanks for working on this!

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Priority: Normal » Minor
pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
StatusFileSize
new code_style_node_module_2.patch10.16 KB

so than, this is also wrong in the patch:

@@ -950,8 +949,6 @@ function node_delete($nid) {
...
 function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {
-  $node = (object)$node;
-

it would qualify as an api change, so yes.

patch fixed
also removed the linefeed problems mentioned in #6

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
StatusFileSize
new code_style_node_module_3.patch10.58 KB

Three more minor issues in node.module:

  • Remove double newline between two functions
  • Correct TODO tag
  • Add basic phpdoc for node_init

The rest looked good, don't want to rtbc this myself, cause I added some more changes, but I guess the next one reviewing this could move forward.

webernet’s picture

webernet commented
Status: Needs review » Needs work

No longer applies.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
StatusFileSize
new code_style_node_module_13.patch10.58 KB

Rerolled.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Status: Needs work » Needs review
moshe weitzman’s picture

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

tested and code reviewed.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

I am not committing a patch with the following in it:

+/**
+ * @todo phpdoc
+ */
pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Assigned: pasqualle » Unassigned
pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Status: Needs work » Needs review
StatusFileSize
new code_style_node_module_18.patch13.2 KB

I added the missing PHPdoc for node_admin_nodes(). Also I improved PHPdocs for node_admin_nodes_validate() and node_admin_nodes_submit() and moved both right after node_admin_nodes(), the form they belong to. Is it rtbc now?

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs review » Reviewed & tested by the community

there are 2 empty lines before

/**
  * Theme node administration overview.

but it does not stop me to mark it rtbc

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Committed. Now may I ask you to move your efforts to help fixing critical bugs?
http://drupal.org/project/issues?projects=3060&categories=bug,task&prior...

Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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