Properly use @see

Then I guess this should be added to Coder's review rules...

I'd like to add this to coder's comment review. I spent 10 minutes trying to write the rule for this, but since drumm is being very specific here, there appear to actually be several rules. If one of you wants to write the regex, I'd be happy to make sure that the coder rules syntax is correct. What coder needs, is the regex of non-conforming code. See http://drupal.org/node/144172 for help on writing rules.

Hi,

I've added in @see usage rules/checks to the 5.x and 6.x versions of the coder module. It ensures that the following conditions are met:

• @see should always be at the beginning of a line, never inline in other comments.
• Always follow function names with ().
• Separate multiple references with ', '
• No trailing punctuation, this is not a sentence.

Please download the dev version and test it.

The only condition not checked is "Keep the references on the same line.". If someone wants to provide me a rule that does this, I'd be happy to try it out. See http://drupal.org/node/144172 for help on writing rules.

Cheers,
Stella

Stella, did you forget to attach the patch?

You can try the attached patch or the latest 5.x-2.x-dev version.

Cheers,
Stella

Coder is now identifying quite a few @see problems, many of which may be false positives. I think that @see template.tpl.php is valid.

If that is the case, then requirements above are incorrect. Drumm - can you clarify the requirements for us?

Cheers,
Stella

All valid:

/**
 * @see module_function()
 * @see module.file.inc
 * @see module_function(), module_invoke(), module.file.inc, file.inc
 */
$foo = bar();
// @see module_function()
$foo->bar = baz();

All invalid:

/**
 * This is special; @see module_function()
 * @see module_function
 * @see module.file.inc,
 *   module_function()
 * @see module_function() module_invoke() module.file.inc file.inc
 * @see module_function().
 * @see module_function(), module_invoke(), module.file.inc, file.inc.
 */
$foo = bar();
// This is special; @see module_function()
$foo->bar = baz();

Dunno, whether @see in single-line comments are actually parsed, but they are perfectly valid, too, so the same rules should apply.

Please try out the attached patch. The previous one has already been checked into CVS, so this one has an additional check for:

@see filename.inc

It will just accept filenames that end in .php or .inc, so as to distinguish it from a normal string or a function name without a trailing (). If this is incorrect, please let me know.

Cheers,
Stella

Thanks heaps!
I like @see

I generally use it to (at least) refer back to the hook_***() definition when implimenting, or to the parent routine when making a trivial helper func.
Sometimes to link related form API funcs - (formname, formname_validate, formname_submit, formname_theme)
Often when I steal code from core to over-ride in my themes (over-ride of @see theme_menu_item() )

Any other suggestions folk think is good practice?

@stella: A developer may reference to any file, including:

/**
 * @see README.txt
 * @see example.module
 * @see mytemplate.tpl.php
 * @see myinclude.inc
 * @see example.ini
 */

In general, function names cannot contain dots, but filenames usually have at least one.

Ok, thanks for the clarification. The attached patch assumes that if it doesn't have a . then it's a filename, otherwise it's a function and must be followed by ().

However I've just thought of another case. Is the following a valid scenario (ignore spacing)?

/**
 * @see myexample.php, some_function()
 *
 */

Cheers,
Stella

Yes, multiple references on one line are valid, and, listing a file in front of a function is valid, too.

Thoughts:
- There should be exactly one space behind @see (so \s* should simply be )
- Since multiple values are allowed, we might need a look-behind to force matching of a dot for filenames, because the dot in [a-zA-Z_\.] is optional and thus, would also match a function name.

'^\@see (([a-zA-Z_]+\(\)|[a-zA-Z_\.\-]+\.[a-zA-Z])(, )?)+'

I think the first condition [a-zA-Z_]+\(\) matches a valid function name.

Try the attached patch. I made a couple of changes.

• Numeric characters weren't being matched in filenames or function names and should be valid.
• Previous patches only alerted on invalid function/file names for the first reference on a line.
• It can now handle mixed function and filename references and tests for a ", " separator.
• It ensures that there is only one space between the @see and the first reference.

I'd appreciate if you could test the patch again.

Cheers,
Stella

Sorry, no review this time. Just wanted to leave a quick note that URLs should be valid for @see, too. A common practice for referencing to d.o issues, PHP bugs, and other pages is

// Force all links to go to a single domain for SEO.
// @see http://drupal.org/node/195366

Ok will look at adding support for this.

Ok try the attached patch, hopefully this one will catch all scenarios!

Cheers,
Stella

In the suggested patch, function names like "foo.bar()" are accepted, which seems incorrect. The "." is accepted for file names, but not for functions.

Marked as duplicates:
#613950: @see tag validation errors
#832694: @see false positives

It's been quite some time, and it looks like the last patch is thoroughly out of date.

I've been trying to fix the whitespace and punctuation errors, with the best fix I can think of so far being to separate the combined check for separators and trailing punctuation. The current code has false positives for trailing punctuation, and I don't think it was catching errors with the separators at all.

Trailing punctuation seems quite simple to check with the regex \w+(\/|\(\))?[\W]$, which will require the @see reference end with word characters optionally followed by '/' (url) or '()' (function), and then no trailing non-word characters.

I am having tremendous difficulty figuring out the separator matching however. I think any invalid instance should be matched with ^\@see\s+.+(,\s\s+|,(?!(\s|$))|(?<!,)\s).+$:
,\s\s+: a comma followed by more than one space
,(?!(\s|$)): a comma not followed by a space (or the end of the line, as to not raise another error for the trailing punctuation situation)
(?<!,)\s: a space not preceded by a comma

I tested the first two segments with http://regexpal.com/ successfully, however javascript regex doesn't support negative look behind so I wasn't able to try the third segment on its own.

While the negative look behind in the third segment could potentially cause problems, none of them individually detect the errors they should properly. Even using only the first, relatively simple, segment (^\@see\s+.+(,\s\s+).+$) causes all @see statements to emit a warning, even if they don't contain a comma!

#1155868: Remove or invert complains about @see in inline comments has been marked as duplicate.

Apparently, it seems like @drumm's first bullet point in the OP was misinterpreted and Coder now complains about @see being used in inline comments; i.e.,

  // Foo is special here, because of bar.
  // @see baz()
  if ($foo != $bar) {
    ...

Very embarrassing.

The only counter-argument against usage of @see in inline comments is that API module doesn't parse it like @see in phpDoc blocks.

My answer to that is: And...?

Proper usage of @see is documented, everyone knows it. It solidly replaces all possible custom phrases and total bloat like "See also foo for more information."

If at all, Coder should complain about such BS phrases and suggest to use @see instead.

If we can't get to an agreement here, then simply remove the part of the review rule that complains about @see in inline comments.

Still (?) 3 failing assertions for punctuation and syntax tests. Didn't check whether they also exist without this patch, but intend to fix them.

Sorry, but @gapple apparently failed to state or include in tests for the patch what kind of "false positives" he's actually talking about, so there's nothing remotely to fix here.

The original regex passes on all existing @see assertions, so I've reverted @gapple's changes in this patch. It only fixes the bogus complaint about @see in inline comments and also improves the test result output of individual assertions.

As in the linked duplicate issues, simple statements such as

/**
 * @see some_function()
 */

were causing an error, and I found that some of the included tests were already failing.
Strangely, checking code which was throwing the error previously with 6.x-2.0-beta1 no longer does (with either 2.0-beta1 or current 2.x-dev), so I guess that is no longer an issue.

Which in turn means that this patch is RTBC.

Committed thanks!