Clean up API docs for rdf

Thanks for taking this on!

I took a look at this patch, and I have a couple of concerns:

a)

/**
  * Implements hook_rdf_namespaces().
+ *
+ * @return array
+ *   Supported RDF namespaces.
  */
 function rdf_rdf_namespaces() {

Normally we don't document return values of hooks, since it would just (usually) repeat the documentation of the hook itself. If there is a reason to document the return value (to clarify specifically what this hook implementation is doing), then the documentation should be more explicit. "Supported RDF namespaces" doesn't really tell me the PHP structure of what is being returned -- what's in the array? Is it a list of names, a list of objects, or what? Anyway, unless it's different from the docs in hook_rdf_namespaces(), just leave it out. See http://drupal.org/node/1354#hookimpl for the standard.

b)

+ * @return array
+ *   Array of RDF namespaces.
  */
 function rdf_get_namespaces() {

Here too, maybe some explanation of what type of PHP thing an "RDF namespace" is (or a link to another function that explains it) would be helpful?

c)

- * @param $type
+ * @param string $type

Adding types to parameters and return values is out of scope for this cleanup, and it makes the patch *much* harder to review (I would have to read every function and see if the types are all correct). See instructions on #1310084: [meta] API documentation cleanup sprint for notes on this.

d)

+ * Builds an array of RDFa attributes for a given mapping.
+ *
+ * Array of RDFa attributes will typically be passed through
+ * drupal_attributes() to create the attributes variables that are available to
+ * template files. These include $attributes, $title_attributes,
+ * $content_attributes and the field-specific $item_attributes variables.

The second paragraph here should probably start with "The" or "This"?

e)

+ * This hook should not be used by modules to alter the bundle mappings. The UI
+ * should always be authoritative. UI mappings are stored in the database and
+ * if hook_entity_info_alter was used to override module defined mappings, it
+ * would override the user defined mapping as well.
+ *
  * @todo May need to move the comment below to another place.
- * This hook should not be used by modules to alter the bundle mappings.
- * The UI should always be authoritative. UI mappings are stored in the
- * database and if hook_entity_info_alter was used to override module defined
- * mappings, it would override the user defined mapping as well.

Although we usually want @todo lines to be at the end of documentation, in this case it refers to the comment that was directly below it, so I think it needs to stay there?

f) This is in rdf.module -- needs fixing in this patch (1st line 80 characters or less):

/**
 * Returns HTML for a template variable wrapped in an HTML element with the
 * RDF attributes.
 *

g) The rdf.test file needs to be fixed in this patch, and isn't included.

tagging

Wouldn't it be optimal to update the docs according to the current api?

for example this patch gives very short descriptions of what the @return values actually are, such as following

/**
  * Implements hook_rdf_namespaces().
+ *
+ * @return array
+ *   Supported RDF namespaces.
  */
function rdf_rdf_namespaces() {

where actually according to the actual api docs we get

An associative array of namespaces where the key is the namespace prefix and the value is the namespace URI.

See doc here

Check http://drupal.org/node/1354#hookimpl for standards for hook implementations. The patch should just omit the return value unless that hook implementation is doing something other than what the hook docs themselves say.

I cleaned up the data types of the @return and @param documentation.
Did what I could on the .test file, since I don't have the extensive knowledge for it. one line :-)

Hopefully this was nice.

#6: drupal-1418980-6-rdf_module_docs_cleanup.patch queued for re-testing.

It could be the patch is failing to apply due to some stuff about binary files right at the top of the patch?

Testbot was unable to apply the patch, so retesting it won't help in this case. The patch probably needs to be rerolled against the latest branch tip. Thanks!

Oops, crosspost.

diff --git a/core/modules/rdf/.rdf.module.swo b/core/modules/rdf/.rdf.module.swo
new file mode 100644
index 0000000..da63230
Binary files /dev/null and b/core/modules/rdf/.rdf.module.swo differ
diff --git a/core/modules/rdf/.rdf.module.swp b/core/modules/rdf/.rdf.module.swp
new file mode 100644
index 0000000..7a78caf
Binary files /dev/null and b/core/modules/rdf/.rdf.module.swp differ

please do not include these files

+++ b/core/modules/rdf/rdf.test
@@ -5,6 +5,10 @@
+/**
+ * Represents a prepared statement
+ */

???

#14: drupal-1418980-14-rdf-api-cleanup.patch queued for re-testing.

Thanks! This is mostly very good. A couple small things to fix:

a) rdf.module

@@ -283,11 +285,14 @@ function rdf_mapping_delete($type, $bundle) {
 ...
  * @return
- *   An array containing RDFa attributes suitable for drupal_attributes().
- */
+ *   RDFa attributes suitable for drupal_attributes().
+ *
+ * @see theme_rdf_template_variable_wrapper()
+*/
 function rdf_rdfa_attributes($mapping, $data = NULL) {

(right at the bottom)

b) rdf.module

@@ -359,11 +364,12 @@ function rdf_modules_uninstalled($modules) {
  *
  * Adds the proper RDF mapping to each entity type/bundle pair.
  *
+ * This hook should not be used by modules to alter the bundle mappings. The UI
+ * should always be authoritative. UI mappings are stored in the database and
+ * if hook_entity_info_alter was used to override module defined mappings, it
+ * would override the user defined mapping as well.
+ *
  * @todo May need to move the comment below to another place.
- * This hook should not be used by modules to alter the bundle mappings.
- * The UI should always be authoritative. UI mappings are stored in the
- * database and if hook_entity_info_alter was used to override module defined
- * mappings, it would override the user defined mapping as well.

I think that the @todo is referring to the comment just below the @todo line, so I would not move it to the bottom of the doc block. :)

c) rdf.test

+/**
+ * Test CRUD functions for RDF mappings.
+ */
 class RdfCrudTestCase extends DrupalWebTestCase {

Test -> Tests

d) rdf.test

@@ -278,13 +281,16 @@ class RdfCrudTestCase extends DrupalWebTestCase {
 ...
   public static function getInfo() {
     return array(
       'name' => 'RDF mapping definition functionality',
-      'description' => 'Test the different types of RDF mappings and ensure the proper RDFa markup in included in nodes and user profile pages.',
+      'description' => 'Test the different types of RDF mappings and ensure the proper RDFa markup are included in nodes and user profile pages.',

I think the correction should be "is" not "are".
... proper RDFa markup is included...

+++ b/core/modules/rdf/rdf.module
@@ -283,11 +285,14 @@ function rdf_mapping_delete($type, $bundle) {
- *   An array containing RDFa attributes suitable for drupal_attributes().
- */
+ *   RDFa attributes suitable for drupal_attributes().
+ *
+ * @see theme_rdf_template_variable_wrapper()
+*/

Just in case it is not very obvious in the output above and in @jhodgdon's, the last line is missing a space at the beginning - if you use dreditor you will see it clearly, highlighted in blue.

Recreated patch per #18

Looks good to me this time.

Thanks @scor and @hansyg! I found a couple of minor issues:

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -252,7 +254,7 @@ function rdf_mapping_save($mapping) {
+ *   Return TRUE if mapping is deleted, FALSE if not.

The word "Return" should be removed here. Also, we need an article (the mapping).

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -360,10 +365,11 @@ function rdf_modules_uninstalled($modules) {
+ * if hook_entity_info_alter was used to override module defined mappings, it

hook_entity_info_alter() should have parens here.

Thanks @kotnik! Those changes look correct and I think everything in the patch itself looks fine now.

I just applied the patch locally and checked through quickly, and it seems a few things are still missing. For example, in rdf.test, RdfMappingHookTestCase still does not have a docblock, and several docblocks begin with "Test" rather than "Tests." Many functions in rdf.module also do not have summaries that follow the standard.

I'll give this a try

Attached patch incorporates comments from #24.

I suppose rdf.test has been removed from the latest 8.x

All of the former .test files have, over the past couple of weeks, been moved into files (one class per file) under core/modules/(module_name)/lib/... in each module. So you probably need to look at that sub-directory and apply the changes that were previously part of rdf.test to the appropriate files there. It wouldn't hurt if you checked all the files there to see if they need any other updates (otherwise, someone else will have to do it anyway).

I did check those files, and fixed numerous instances of the wrong verb tense, for example. The fixes are included in the patch at #26.

#26: drupal-1418980-26-clean_api_docs_rdf.patch queued for re-testing.

I went ahead and committed the patch above, since everything in it is good and it's been sitting here for a long time. Thanks!

But the RDF module directory needs some further work. For instance in the CommentAttributesTest.php file, I found this:

  /**
   * Tests the presence of the RDFa markup for the title, date and author and
   * homepage on registered users and anonymous comments.
   */
  public function testCommentRdfaMarkup() {

(needs a one-line summary).

So if someone could go through all the tests and fix up the methods, and make a new patch, that would be good! Remember, in tests we don't document getInfo() or setUp() methods (sigh).

allright, here's a new patch with fixes for the files in the lib/Drupal/files/rdf/Tests folder

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/MappingDefinitionTest.php
@@ -50,8 +50,10 @@ class MappingDefinitionTest extends TaxonomyTestBase {
+   * Tests if RDF mapping defined in rdf_test.install is used.
+   * ¶
+   * Creates a content type and a node of type test_bundle_hook_install and ¶

This patch has some whitespace issues visible in dreditor

Got rid of the whitespace issues

what's in the patch looks great to me.

Thanks! Committed to 8.x.

I think it's time to port this whole thing to 7.x now. (two different patches were committed)

Backported to D7.

Note: that was #33 and #24 combined.

+++ b/modules/rdf/rdf.moduleundefined
@@ -283,10 +285,13 @@ function rdf_mapping_delete($type, $bundle) {
  * @param $data
- *   A value that needs to be converted by the provided callback function.
+ *   (optional) A value that needs to be converted by the provided callback
+ *   function.

this seems like it could use a type, or example values. Is it a string?

Also, should say what the default value is... Oh, I guess not. http://drupal.org/node/1354#functions was updated.

+++ b/modules/rdf/rdf.testundefined
@@ -115,6 +115,8 @@ class RdfRdfaMarkupTestCase extends DrupalWebTestCase {
+   * Tests if file fields in teasers have correct resources.
+   * ¶

@@ -308,8 +310,10 @@ class RdfMappingDefinitionTestCase extends TaxonomyWebTestCase {
+   * ¶
+   * Creates a content type and a node of type test_bundle_hook_install and ¶

there are a few places that have an extra space at the end of lines.

Point 1 in #39 is out of scope for this issue per #2 (point c). The whitespace needs to go, though :)

Hi

I tried to remove the white spaces. I have a fresh D8 clone. The patch file #37 shows file
diff --git a/modules/rdf/rdf.test b/modules/rdf/rdf.test but in my RDF module there is no file called as rdf.test but it contains a test folder. Please correct me i am wrong.

This is a Drupal 7.x issue -- maybe that is the problem?

Sidharthap. As jhodgdon mentioned you'll need to clone out Drupal 7 for this issue as its a backport.

You can follow the same instructions you did for 8 but changing it to 7:
http://drupal.org/node/3060/git-instructions/7.x

Install that and then work on your patch from there.

Thank you @Kbasarab, @Jhodgdon, Here is the patch file.

#45: rdf.1418980.45.docs-cleanup.patch queued for re-testing.

Oh gracious, this has been around for a long time -- sorry for the lack of reviews!

This all looks good except in rdf.test, several functions need a blank line inserted after the first documentation line, such as:

   /**
+   * Tests if file fields in teasers have correct resources.
    * Ensure that file fields have the correct resource as the object in RDFa

There are at least 5 methods with this same problem.

Fixed name

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!