We should clean up the documentation to help with on-boarding additional developers.

CommentFileSizeAuthor
#1 edit-improve-documentation-1672598.patch20.17 KBpsynaptic

Comments

psynaptic’s picture

psynaptic commented
Status: Active » Needs review
StatusFileSize
new edit-improve-documentation-1672598.patch20.17 KB

Patch attached.

I have purposely avoided adding fuller doxygen function header comments describing function parameters and return values since I expect to change the names of some of those arguments in another patch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Fixed 
+++ b/edit.moduleundefined
@@ -126,13 +135,13 @@ function edit_form_fape_field_edit_form_alter(&$form, &$form_state) {
+    // Pseudo-fields that are language-independent, such as title, name and date.

I wasn't sure here if that dash was needed (not being a native speaker), thanks for the correction :)

+++ b/edit.moduleundefined
@@ -173,12 +183,12 @@ function edit_field_formatter_info_alter(&$info) {
- * Implements of hook_preprocess_HOOK().
+ * Implements hook_preprocess_page().

"Implements of" — OMFG EPIC WIMFAIL

+++ b/includes/fape.incundefined
@@ -1,12 +1,15 @@
-// This will hopefully be accepted into the FAPE module: http://drupal.org/node/1650442.
...
- * Subform to edit the entity 'author' field.
+ * Generates a sub-form for editing the entity's "author" field.

There is no point in updating the docs of the FAPE include, because:
1) it's going away as soon as http://drupal.org/node/1650442 gets committed
2) it's analogous to how the FAPE module documents things; if you want to change this, you'll have to change all of the FAPE module's code docs as well, I'm afraid :(

+++ b/includes/pages.incundefined
@@ -1,10 +1,14 @@
+ * Integrates Panels IPE (In-Place Editor) module with Edit module.

This is not true. We're leveraging FAPE just to generate forms. FAPE is independent of Panels, and Edit definitely is.

+++ b/includes/toolbar.incundefined
@@ -1,9 +1,10 @@
+ * Integrates the Toolbar module with Edit module.

I'd say it's the other way around. Or even: we're *overriding* the default behavior.

So, I committed:
- all of the ipe.module changes
- none of the includes/fape.inc changes
- all of the includes/pages.inc changes, with corrections for the Panels IPE mentions
- all of the includes/toolbar.inc changes, with the above remark applied

http://drupalcode.org/project/edit.git/commit/8238c54

Thanks! :)

psynaptic’s picture

psynaptic commented

There is no point in updating the docs of the FAPE include, because:
1) it's going away as soon as http://drupal.org/node/1650442 gets committed
2) it's analogous to how the FAPE module documents things; if you want to change this, you'll have to change all of the FAPE module's code docs as well, I'm afraid :(

The patched in the FAPE issue doesn't follow Drupal's coding standards so needs a bit of work before it would be committed. I checked fape.module and it doesn't use the same formatting for doxygen function headers or file header as the FAPE include in this module. I think we should uphold Drupal's standards whether something is likely to go away soon or not.

I would agree that we don't need to necessarily commit these changes though, as they are like that in FAPE module.

/**
- * Subform to edit the entity 'created' field.
+ * Generates a sub-form to edit the entity's "created" field.
  *
- * This isn't a true form. As such it modifies the $form by reference.
+ * This isn't a true form; it modifies the form by reference.
  */

However, I'm happy to change both fape.module and this include if the changes are deemed good.

Thanks for the other corrections. They all seem good to me.

psynaptic’s picture

psynaptic commented

I'm working on reworking the patch from #1650442: Support node:author and node:created now.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Cool :)

Status: Fixed » Closed (fixed)

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