Hi!

Congrats on the module. He is excellent.

Just a few coding standards improvements.

Thanks.

Fix: field_collection-codingstandards-2833211-2.patch

Comments

renatog created an issue. See original summary.

renatog’s picture

Assigned: renatog » Unassigned
Status: Active » Needs review
StatusFileSize
new58.74 KB

Patch submitted with the fixes.

Thanks.

Regards.

renatog’s picture

Issue summary: View changes
StatusFileSize
new27.22 KB
cebasqueira’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new26.45 KB

RTBC

jmuzz’s picture

Status: Reviewed & tested by the community » Needs work

I like where this is going but there's a few issues.

+   * @codingStandardsIgnoreStart
[...]
+  // @codingStandardsIgnoreEnd

Can we leave these out? They aren't in core and most of the popular contrib modules don't have them. I saw them in the date module but they weren't used like this.

I don't understand why they would be needed for the object properties or the short object method declarations.

For the long function declarations I am in favour of splitting them to multiple lines but that may not satisfy the analysing script or actual coding standards. Example:

function long_name(
  $lots, $of, $arguments)
{
...
}
- * entity_update_7003 will attempt to install entitycache tables for existing
+ * Entity_update_7003 will attempt to install entitycache tables for existing

Comments referring to functions should use the same casing as the function definition.

 /**
- * #pre_render callback that ensures the element is rendered as being required.
+ * Pre_render callback that ensures the element is rendered as being required.
  */

It is a #pre_render callback.

- * field_collections tests.
+ * Field_collections tests.

I'd change it to "Field collection tests." probably.

+  /**
+   * Function to Get Info.
+   */

The one liners should state what the function does, not what it is. For example "Gets the Info".

The way the standards document explains it is: For most functions [...] summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."

There are actually a lot of other small violations of this such as not using third person like "Test something" when it should be "Tests something" and I'd still commit this with them in it. Could open up a followup issue for that rule specifically.

-@charset "UTF-8";
-
-.field-collection-container {
-  border-bottom: 1px solid #D3D7D9;
+@charset "UTF-8"; .field-collection-container {
+  border-bottom: 1px solid #d3d7d9;

Not sure you meant to put the @charset on the same line as other stuff. It should have its own line.

danilo_formagio’s picture

Assigned: danilo_formagio » Unassigned
ram4nd’s picture

Status: Needs work » Closed (outdated)
Issue tags: -#ciandt-contrib