? Success

User tests: Successful: Unsuccessful:

avatar wojsmol
wojsmol
16 Jun 2016

Pull Request for improvment .

Summary of Changes

  • modify PEAR.Functions.ValidDefaultValue exclude pattern
  • removed Generic.CodeAnalysis.UselessOverridingMethod for modules folder

Testing Instructions

Check if travis still passes

Additional comments

I modified the exclusion for PEAR.Functions.ValidDefaultValue exclude pattern due to the result of this travis build https://travis-ci.org/wojsmol/joomla-cms/jobs/137958211

cc @wilsonge @photodude

avatar wojsmol wojsmol - open - 16 Jun 2016
avatar wojsmol wojsmol - change - 16 Jun 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jun 2016
Labels Added: ?
avatar wojsmol wojsmol - change - 16 Jun 2016
The description was changed
avatar brianteeman brianteeman - change - 16 Jun 2016
Category Code style
avatar photodude
photodude - comment - 16 Jun 2016

Looks like there was a couple of things missed between the clean up of the phpcs 1.5.x rules and the PHPCS 2.x rules. Here is what should happen based on the decisions from the phpcs2.x development.

  • Remove Generic.CodeAnalysis.UselessOverridingMethod, This sniff has too many false positives, and will need a "smarter" sniff to handle methods which are just calling the parent method with a change to the default arguments.

As for PEAR.Functions.ValidDefaultValue, that exclusion is needed. Historically we have included that sniff. Although we don't have it written out that function "Arguments with default values must be at the end of the argument list"; we do extend from the PEAR standard. I'm inclined to say that we should refactor that function or exclude the single file that's causing issues. Another option would be to implement an exclusion for PEAR.Functions.ValidDefaultValue.NotAtEnd if that is determined to be one of our deviations from the PEAR standard.

avatar wojsmol
wojsmol - comment - 16 Jun 2016

@photodude Generic.CodeAnalysis.UselessOverridingMethod - should I revert commit b23cc64 ?
PEAR.Functions.ValidDefaultValue - in commit 8af941c has implemented the exclusion for a single file.

avatar photodude
photodude - comment - 16 Jun 2016

I don't think that other comit needs to be reverted, just remove Generic.CodeAnalysis.UselessOverridingMethod from the ruleset with this PR as explained above. It was an oversite that it it didn't get removed before.

The other change for PEAR.Functions.ValidDefaultValue is an appropreate solution for now. Long term we will need to decide if that is the most approreate solution or choose one of the other options I suggested.

avatar wojsmol
wojsmol - comment - 16 Jun 2016

@photodude done

avatar photodude
photodude - comment - 16 Jun 2016

These corrections look good to me. @wilsonge

avatar wojsmol
wojsmol - comment - 29 Jun 2016
avatar zero-24 zero-24 - change - 29 Jun 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 29 Jun 2016

RTC based on the green mark by travis Thanks ;)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10834.

avatar joomla-cms-bot joomla-cms-bot - change - 29 Jun 2016
Labels Added: ?
avatar zero-24
zero-24 - comment - 29 Jun 2016

btw: what is wrong with modules/mod_articles_category/helper.php?

avatar wilsonge wilsonge - change - 29 Jun 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-06-29 10:50:44
Closed_By wilsonge
avatar wilsonge wilsonge - close - 29 Jun 2016
avatar wilsonge wilsonge - merge - 29 Jun 2016
avatar joomla-cms-bot joomla-cms-bot - close - 29 Jun 2016
avatar wilsonge wilsonge - reference | 06d6640 - 29 Jun 16
avatar wilsonge wilsonge - merge - 29 Jun 2016
avatar wilsonge wilsonge - close - 29 Jun 2016
avatar wilsonge wilsonge - change - 29 Jun 2016
Milestone Added:
avatar joomla-cms-bot joomla-cms-bot - change - 29 Jun 2016
Labels Removed: ?
avatar wojsmol wojsmol - head_ref_deleted - 29 Jun 2016

Add a Comment

Login with GitHub to post a comment