How to "ignore" warnings

I've always considered lint a model. I don't know where the official lint documentation lives, but I just found some documentation as http://www.thinkage.ca/english/gcos/expl/lint/manu/manu.html. This paragraph seems very appropriate

"Before we begin, we should note that it is inevitable that LINT will miss some problems and will also complain about code that turns out to be valid. The designers have tried to chart a course between too much laxness (which may miss significant irregularities) and too much nit-picking (which produces quantities of irrelevant output that you will likely ignore). Thus we have designed this version of LINT to report situations that are usually signs of errors. While odd constructions and unusual programming style can confuse LINT from time to time, the majority of material that LINT locates should deserve the programmer's attention."

Lint use's inline comments to help it, and I still frequently use the "// FALLTHROUGH" comment just because it's a good comment IMO. I would prefer the inline comment method to ignore coder warnings, just because it will be easier to maintain. Rather than a warning number or hashes though, I would prefer a readable short name. But I have always been against the idea that people should change their code just because coder says something.

The comment above that says "I would position Coder as a helpful tool — a friend rather than a nasty drill sergeant — that gracefully steps aside when it reaches its limitations." ... might mean that coder has grown beyond it's initial usage. I haven't spoken at a DrupalCon about coder in a long time (maybe four years), but when I did, I always said that "You are smarter than coder" and you should ignore warnings that you knew were wrong. The lint paragraph I quoted above says it best. That said, ... I don't want coder to be a Drill Sergeant, but given it's widespread usage, and the desire to run it on qa.d.o, I do think it's time to follow the lint model one step further and add inline comments.

I've taken a stab at a new ignore system. Every rule now has a rule name, by default this is the array index, so by default it's a number. But the rule names can be set. We can either stay with the numbering system, or go through all of the reviews and give them mnemonic names. If we stick with the numbering system, we'll just have to make sure that all new rules are added to the end.

Then in the code, above the line you want to ignore, add a comment like:

// @ignore upgrade7x_74

I've added ignores to coder itself so others can see how to define them.

Is the string "@ignore" appropriate, or should we use something else? I like the @ sign directory, but could see using # if this confuses doxygen. I played with using @coder or @coder-ignore or @coder-review and concluded that @ignore was more readable.

See http://drupalcode.org/project/coder.git/commit/d888ab6

Note that the above will also handle the Drupal Code Sniffer "sniffer" review inside coder and properly ignore warnings.

That looks great! I never intended the original ignore system to be the final version, just a proof of concept more than anything else. I didn't go with the comment method as I didn't want to deal with the lack of comments for certain #sources.

+1 for inline comments and +1 for @ignore rather than @coder-ignore or anything else, it's short and sweet and quite clear what it does.

However, from looking at the code committed I see that the // @ignore comment is on the line before the line to be ignored. I had kinda expected it to be on the same line, but as long as it's documented, that's fine with me.

Just a few questions:

1. Have you tested this method for rules where #source is set to 'quote', etc?
2. How do you handle multi-line reviews or multi-line ignores? Though I guess the multi-line reviews need to be overhauled and you may not want to support multi-line ignores.

Also, it'd be great if we could add simpletests for this.

A few thoughts:

- // @ignore comments will not bother Doxygen. Doxygen and the API module are only parsing comments in /** */.

- +1 for putting it on the line before, which is our standard for comments in general... possibly could it be:

// @ignore rule_name N

to ignore the next N lines?

- THIS SOUNDS GREAT!!!! And it's definitely needed. I've just resorted to putting normal comments in my code in my contrib modules for places like this, to remind myself to ignore the warnings and not change anything, but of course the warnings still come up.

I'll implement whatever the community agrees to. But the way I wrote this code, it actually ignores it on the current line and the next line. I was concerned that inline comments on the current line is actually against our current coding standards. And if we add // @ignore on that line it would generate it's own coding standards violations.

I agree totally that comments on the same line violate our usual coding standards.

So, would it be possible to do:

  // @ignore upgrade7x_74
  (1 line of code that violates this here)

or

  // @ignore upgrade7x_74 3
  (3 lines of code that violate this here)

Yes, are there better rule names we could use?

I'm against the idea of having the // @ignore comments at the end of the line -- it makes them harder to see/scan if the line of code is long. Also if we have the multi-line option, it makes them not very uniform... I think it's easier to remember the coding standard if it says "Put it on the line before in all cases" than "Sometimes this and sometimes that", and I'm definitely all for simpler coding standards rather than having a lot of conditionals and exceptions.

@jhodgdon, yes this would be possible:

// @ignore rule_name N

But I'd prefer to use a semi-colon, because currently whitespace or commas allow multiple rules ignores, as-in:

// @ignore rule_name1, rule_name2, rule_name3

What do you think of the following, where no :N implies this line and the next line.

// @ignore rule_name1:N1, rule_name2:N2, rule_name3:N3

@salvis, I too would like more readable names. And I would likely commit a change that someone else made. But I'm really loath to personally go through all of our old upgrade reviews and assigning names. There are 325 current reviews in coder, not including contrib modules that implement hook_coder (such as coder tough love). I've already converted the few cases where a name is required (in rules callbacks that generate several different warnings, all have the same rules index number, and thus need an explicit name).

$ grep '$rules\[\]' includes/* | wc -l
     325

Also, I don't think we can give explicit names to the sniffer rules, these have to rely on some numbering. I choose an md5 hash of the string. For example, sniffer_83576421b5a32494aa5b58e538d68370 is the rule that says "Missing parameter type at position 1". Of course, there will be a different hash the warning says "Missing parameter type at position 2." Thus, if people start relying on DCS and start adding ignores for DCS, these are going to have hashes in them. And FWIW, the name of this review is currently "sniffer". I'm open to other names, like DCS or codesniffer. I didn't like DCS because I don't think it has any name recognition yet, but if I'm wrong, that's probably what it should be called.

Two more thoughts: (a) if we're going to ask people to add @ignore's to their code, I/we/coder can give reasonable looking @ignore names, so I think we should fix those 325 rules, (b) I personally think adding it to the line above is fine, this looks a lot like a lint pragma statement to me, it's a "pre-processor" to me, but honestly, we need to get many more core developers to weigh in on their preference before making a decision for core.

IMO, before doing anything here, it would be best to get @dries, @webchick, @catch, @gabor, @crell, @chx, and probably a few others to render an opinion.

I've asked @klausi to comment here. I'm concerned that putting inline comments on the same line as the ignore will cause problems with DCS when run outside the coder framework, such as from eclipse.

The suggestion for N is really about scope. So this is what I propose,

// @ignore rule[:scope], ...

Where rule is a mnemonic rule name and scope is either "file", "function", "comment", or any numeric number representing a number of lines. By default, the scope applies to the current line and the next line. To make it apply to the current line and the next two lines, you'd use :2.

The ... implies that you can put as many of these on a line as you like.

After a nights sleep, I now feel that if core wants to add ignore rules, or how they want to add them, seems like a separate issue. We should implement the above, see how it works, how we feel about it, then suggest it for core. IMO, we shouldn't make any coding standards decisions right now. But we should implement something, try it out in contrib in a few modules, possibly tweak it based on that experience, then offer a solution to core.

Marking as "needs work" to indicate I need to implement the above.

I've implemented the scoping logic for function, file, class, comment, and a numeric number of lines. It still needs tests (and tests currently seem a bit broken --- ugh, they were working just a couple days ago, sorry).

+1 on #18 -- and I think you are right that you can do this in Coder and Core can decide whether it wants these things in the code (I think they will, since there's an effort in place to get Core to pass coder review, and there will always be exceptions).

Also +1 on the idea in #13: making the machine names of the rules more human-readable.

Thanks douggreen for taking this on!

PHP_Codesniffer has this syntax:

$xmlPackage = new XMLPackage;
// @codingStandardsIgnoreStart
$xmlPackage['error_code'] = get_default_error_code_value();
// @codingStandardsIgnoreEnd
$xmlPackage->send();

That completely ignores all sniffs on the part between the tags. See also http://pear.php.net/manual/en/package.php.php-codesniffer.advanced-usage...

PHP_CodeSniffer just introduced something interesting in 1.3.6, see http://pear.php.net/package/PHP_CodeSniffer/download/

// @codingStandardsChangeSetting Drupal.WhiteSpace.ScopeIndent indent 4
... stuff with 4 space indentation here ...
// change it back afterwards
// @codingStandardsChangeSetting Drupal.WhiteSpace.ScopeIndent indent 2

Very handy. At least for drupal code sniffer stuff we can just use that right away.

A few quick examples:

This is comment scope:

// @ignore rule:comment
/*
 * Let's assume that this line is OK
 *
 * But way down here, maybe,
 * This is a comment that has some valid stuff in it that coder flags................................
 **/

This would also work for coder, but would likely have doxygen problems:

/*
 * @ignore rule:comment
 * Let's assume that this line is OK
 *
 * But way down here, maybe,
 * This is a comment that has some valid stuff in it that coder flags................................
 **/

This is a function scope:

$a = 'nothing ignored yet';

/**
 * My function
 */
// @ignore rule:function
function my_function() {
  $b = 'the rule is ignored';
}

$c = 'the rule is no longer ignored';

function your_function() {
  $d = 'of course the rule is not ignored here either';
}

You can also enable function (or class scope) from anywhere within the function and class.

class MyClass {
  protected $a = 'nothing is ignored yet';

  function myFunction() {
    $b = 'the rule is not ignored here';
  }

  // @ignore rule:class

  protected function yourFunction() {
    $b = 'the rule is ignored here';
  }

  protected $c = 'the rule is also ignored here';
};

function another_function() {
  $d = 'the rule is not ignored here because we are outside the class now';
}

This example could definitely be a problem for the API module:

/**
* My function
*/
// @ignore rule:function
function my_function() {
  $b = 'the rule is ignored';
}

$c = 'the rule is no longer ignored';

function your_function() {
  $d = 'of course the rule is not ignored here either';
}

because I think we need the /** */ comment to be directly before the function declaration, with nothing in between.

So even if it works for Coder, I would be in favor of advocating that people put the @ignore rule:function comment somewhere inside the function body (preferably at the top, just after the declaration line. Similarly for classes -- suggest that people put it at the top. (I realize Coder might be fine either way, but let's also consider that human readers might want to know about this and suggest people adopt a good practice).

wfm, I think I've implemented something flexible, that's not too expensive on the lexical analysis, and you are welcome to come up with any standards you like ;) Once you set those standards, we can even make coder enforce them!

I've just committed some tests. I haven't converted all of the rules numbers to mnemonics. But I've converted enough of them that someone else can probably complete the task. I've done it for druplart, production, sql, and style. So ... anyone wishing to test this, just be aware that some of the rules names will change when this is done.

I had some trouble figuring out how to use this feature. If anyone else is having problems, you can take a look at this thread: #1784954: Put the 2.x-dev snapshot on the project homepage

Cheers,

Albert.

I just changed the sniffer ignore names from an md5 hash to something based on the sniffer source.

This makes much more readable names, such as:

 +69: [sniffer_array_array_longlinedeclaration, minor] If the line declaring an array spans longer than 80 characters, each element should be broken into its own line
 +113: [sniffer_whitespace_scopeindent_incorrectexact, minor] Line indented incorrectly; expected 2 spaces, found 0

See http://drupalcode.org/project/coder.git/commit/2671dc8

This should also handle @ignore in the handling of .info files: reopening because this was missed in the discussion above. Sample issue: the spurious upgrade7x_1 warning about listing all files.

Ignore currently does not work for http://drupal.org/project/coder_tough_love plugin. But shouldn't this functionality be built into review module itself, without any need for plugin-site support?

Are @ignore rules at a place where this document can be updated? See the section on "Ignore warnings". I don't mind helping do that, but I am not 100% sure I know all of the possible formats, thought I am using the most simple iteration in a project right now, @ignore .

Just feedback to the maintainers that I have just discovered this @ignore discussion, and it may help me address my concerns recently reported in many other Coder issues
(reduce the annoyance factor for those rules I don't agree with, and for those rules not yet well tuned for object-oriented code), Webel

Coder 7.x is frozen now and will not receive updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.