? ? Pending

User tests: Successful: Unsuccessful:

avatar photodude
photodude
1 Jun 2018

Pull Request for Issue code style corrections for doc comment types. continues work from #20522

Summary of Changes

PHPCS2 manual fixes

  • Variables passed by reference should not have the & prefixed in the doc comment
    • Joomla.Commenting.FunctionComment.MissingParamTag
    • Joomla.Commenting.FunctionComment.ParamNameNoMatch
    • Doc comment for a parameter does not match the actual variable name
  • Comment closer must be on a new line
    • just convert to single line comment
  • be more specific with the PHPCS rules we want to exclude

Found using the Joomla code standards 2.0.0 PHPCS2-RC

Testing Instructions

Merge by code review

Expected result

code style has been applied as listed above, old code style testing on drone does not error.

Actual result

code style had not been applied. Found using the Joomla code standards 2.0.0 PHPCS2-RC

Documentation Changes Required

none

avatar photodude photodude - open - 1 Jun 2018
avatar photodude photodude - change - 1 Jun 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jun 2018
Category Administration com_finder com_installer com_menus Repository
avatar photodude photodude - change - 1 Jun 2018
The description was changed
avatar photodude photodude - edited - 1 Jun 2018
avatar photodude photodude - change - 1 Jun 2018
Labels Added: ?
avatar photodude
photodude - comment - 1 Jun 2018

@mbabker @wilsonge @zero-24 @Quy
For easy review purposes, and to avoid large scale merge conflicts, I've been trying to keep these PR's between 15-25 files; Please let me know if you would like to see PR's with more files to help move the PHPCS 2 update project along faster.

With the current modified exclusion list I'm working from I have A TOTAL OF 809 ERRORS AND 1 WARNING WERE FOUND IN 265 FILES where PHPCBF CAN FIX 169 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
output.txt

avatar Quy
Quy - comment - 1 Jun 2018

I have tested this item successfully on 98f7f16


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

avatar Quy Quy - test_item - 1 Jun 2018 - Tested successfully
avatar laoneo
laoneo - comment - 2 Jun 2018

Can we not make this kind of pr's against 4? We are very far behind merging staging into 4.0-dev and such pr's make it even harder for @wilsonge to keep the branches in sync. Don't get me wrong it is good work.

avatar photodude
photodude - comment - 2 Jun 2018

@laoneo The original plan was to get 3.x on PHPCS2 which requires cleaning up some code style items to make sure everything is working correctly, then work on getting J4 running on PHPCS3+. see the last few comments of joomla/coding-standards#143
We have found an issue with anonymous classes and closures indenting that needs to be addressed before we can finalize either of the updates. see joomla/coding-standards#230

We could consider large execution lists for both J3 and J4 to implement the updates now, but it increases the probability that the exclusions become permanent and somethings will not be up to the code standards.

avatar mbabker
mbabker - comment - 2 Jun 2018

Regardless of whether these are merged to staging or 4.0-dev it still creates merge conflicts. It's not like merging to only one branch is going to magically make things easier.

avatar carlitorweb carlitorweb - test_item - 7 Jun 2018 - Tested successfully
avatar carlitorweb
carlitorweb - comment - 7 Jun 2018

I have tested this item successfully on 98f7f16


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

avatar Quy Quy - change - 7 Jun 2018
Status Pending Ready to Commit
avatar Quy
Quy - comment - 7 Jun 2018

RTC


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

avatar zero-24 zero-24 - change - 8 Jun 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-08 18:26:20
Closed_By zero-24
Labels Added: ?
avatar zero-24 zero-24 - close - 8 Jun 2018
avatar zero-24 zero-24 - merge - 8 Jun 2018
avatar zero-24
zero-24 - comment - 8 Jun 2018

Merged

avatar photodude
photodude - comment - 8 Jun 2018

Thanks Everyone

avatar laoneo
laoneo - comment - 8 Jun 2018

Just that you guys have an idea how hard such pr's do make the life of our release leader:

image

It will become even more worse when all the finder changes of @Hackwar are merged.

avatar mbabker
mbabker - comment - 8 Jun 2018

By the same argument then the privacy tool suite should only merge to 4.0 because THAT is going to create all sorts of merge conflicts too.

Again, it doesn't matter what branch these things land on, they're going to create merge conflicts somewhere and make someone's life miserable. There is no getting around that without hard forking the 4.0 branch (almost the same way 2.5 and 3.x were dealt with after 3.0's release) and not merging up ANY changes from any lower branches.

avatar laoneo
laoneo - comment - 9 Jun 2018

Not sure how a new feature can add that many conflicts. Anyway. What I wanted to express here is that all I try is to organize things a bit for J4 and I see I fail once more. That we are since months over 250 commits behind staging documents our current situation very well.

Don't get me wrong, the CS fixer work is important and I appreciate all what you guys are doing.

I mean you can't blame the project to be an unorganized mess and then on the other hand stop people to work against that. But I see it turned out once more in a nonsense argumentation battle. I will just shut up as I just don't have time for.

avatar mbabker
mbabker - comment - 9 Jun 2018

No drama intended.

The feature creates conflicts in common integration points. Ask anyone who's done work on that repo and had to deal with constantly changing SQL files, that's just one feature repo. Add the SQL changes in 4.0.

As for CS fixes, here's the thing. If you ONLY apply them to 4.0, sooner or later it actually makes dealing with merges more difficult because the code starts diverging so much from the lower branches. If you apply them on lower branches and merge up, sure you're going to run into merge conflicts in that initial merge, but it shouldn't create too many more problems after that unless you're constantly changing those same blocks of code.

I did a lot of CS changes in the Framework's 2.0 branches to adapt to PHP 7 syntactic sugar that wasn't available in PHP 5.3, then had the big move of all the packages to the PHPCS 2 standards. In quite a few packages, branch merging is not a simple thing because of that (especially something like the Filesystem package where we basically changed every snake case variable to camel case in one go). So in the long run, it's more sane for everyone to keep the code structure/style consistent across branches, even if it means there are short term complications. It's already annoying enough having to unconflict 7 or 8 files every time we bump version strings, and nobody likes the "new year new copyright statement" commit.

avatar laoneo
laoneo - comment - 9 Jun 2018

Every pr which can be moved to J4 creates no conflicts at all. I get your argument about the diverge, but when we can change the pr's which are not really needed (I hope this understands now nobody the wrong way) to J4, then I would say it rather motivates people in participating in J4. Just wondering how @wilsonge feels about all of this as he is most affected because he manages the sync.

Add a Comment

Login with GitHub to post a comment