Removing sniff for TODO comments #12

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@ADmad
Member
ADmad commented Oct 28, 2012

I don't think warnings for TODO comments are really needed since most TODOs are not addressed until next major release. Just creates extra noise in the reports.

@AD7six
Member
AD7six commented Oct 28, 2012

I'm not sure about that - it might create noise in the cakephp repo's report, but the standard isn't just for the core.

Generally seeing todo warnings in application cs reports is very useful for highlighting todo tasks that need addressing, and being continually reminded of their existence is a good thing.

@ADmad
Member
ADmad commented Oct 28, 2012

Yeah I was only looking for the core point of view. So I guess no need to remove it. What do you think about the "Code after RETURN statement cannot be executed" warnings though? Early returns are a good thing and used at multiple places in the core. In IRC @markstory too was of the opinion that those warnings shouldn't be there. Though I was unable to find exactly which sniff triggers that warning.

@AD7six
Member
AD7six commented Oct 28, 2012

Where are we seeing them generate false-positives?

Unless the sniff has been changed, that shouldn't be removed either - this does not trigger that warning:

function foo($in) {
    if ($in) {
        return;
    }

    return $in;
}

This will (but it clearly should):

function foo($in) {
      return;

      anything();
  }

It comes from the inherited Squiz/Sniffs/PHP/NonExecutableCodeSniff.php sniff.

@ADmad
Member
ADmad commented Oct 28, 2012

It complains about returns in switch case like here.

@AD7six
Member
AD7six commented Oct 28, 2012

we could patch or override that sniff - it only accounts for return statements in if blocks at the moment.

I'd rather fix it than remove it.

@ADmad
Member
ADmad commented Oct 28, 2012

Yeah the sniff should be fixed, it should account for switch/case too same as if.

@markstory
Member

I'm on the fence about removing the TODO sniff, it might be simpler for us to just remove the bogus todo's in CakePHP. As for the early return, I think we should either remove or fix it. The sniff built-in to phpcs is no longer working as I would expect it.

@ceeram
Member
ceeram commented Nov 14, 2012

I think we can close this if we merge pull request cakephp/cakephp#959

@ADmad ADmad closed this Nov 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment