search

civicrm / coder

Code sniffer configuration for CiviCRM. (Relaxed variant of Drupal CS)
5 stars 11 forks source link

Ensure that finally operator is correctly tagged as opened so that in… #5

Closed seamuslee001 closed 5 years ago

seamuslee001 commented 6 years ago

…denting is set correctly

We have seen just recently we got our first block of code using finally { }

As evidenced by https://test.civicrm.org/job/CiviCRM-Core-PR/18710/checkstyleResult/new/ it seems our current code sniffing doesn't pickup the opening brace of the finally { for some reason.

This adds in the opening brace so it knows to add an extra 2 spaces into the expected indent

ping @totten @colemanw

colemanw commented 6 years ago

@seamuslee001 I know nothing about Coder, but thank you for the patch. @totten probably knows more about this than me. One other thing I recently noticed is that the new square bracket array syntax passes coder sniffing (usually), but I think it's by accident, as certain types of array keys will cause it to fail. How would we find and merge an upstream fix for this (assuming it's been fixed in the past few years since we forked this project)?

totten commented 5 years ago

@seamuslee001 Is this still an issue after the coder upgrade?

I tried linting a sample script with a finally clause, eg


function foo() {
  try {
    ab();
  }
  finally {
    cd();
    if (foo()) {
      bar();
    }
  }
}

It was more forgiving than I expected (wrt to positioning of } and subsequent finally), but this seems to be about indentation of finally, and (with the variants I tried) it seemed reasonably accurate.

totten commented 5 years ago

Closing, because I think it's been addressed by the bigger upgrade, and it's stale anyway. But please reopen if it's still an issue.