? Success

User tests: Successful: Unsuccessful:

avatar wojsmol
wojsmol
29 May 2016

Summary of Changes

Code style changes for com_content

Testing Instructions

code review

cc @andrepereiradasilva @JoomliC

avatar wojsmol wojsmol - open - 29 May 2016
avatar wojsmol wojsmol - change - 29 May 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 May 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 29 May 2016
Category Code style
avatar JoomliC
JoomliC - comment - 29 May 2016

@wojsmol I only checked config.xml, and already 6 errors found...
Could you please do a new checking of all yours changes in this PR ? (i know it's time, but it could save time too on review ;-) )

One question: you started with plugins, then modules, and now components, but not all modules/plugins are reviewed (just did mod_multilangstatus yesterday in the same time of another purpose), so, is there a reason for that ? (code style review is a long and not funny job, but good to be done, but maybe i could help later when i will have finished some other work on modals) ;-) ).
Note: could you not work on com_templates for now, as i have started many changes in many files, and so, i can do the code style review for this component in the same time)

avatar wojsmol
wojsmol - comment - 29 May 2016

@JoomliC I reviewed my changes again. At the moment they are reviewed all the frontend modules and plugins. PR for com_content I have prepared a little earlier than planned in connection with my discussion with JM in PR #10621.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 May 2016

ok please make the changes i identified and for me is ok.

avatar andrepereiradasilva andrepereiradasilva - test_item - 30 May 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 30 May 2016

I have tested this item successfully on c663e06


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

avatar joomla-cms-bot
joomla-cms-bot - comment - 31 May 2016

This PR has received new commits.

CC: @andrepereiradasilva


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 31 May 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 31 May 2016

I have tested this item successfully on ae43910


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

avatar wojsmol
wojsmol - comment - 5 Jun 2016

@JoomliC Please test

avatar JoomliC
JoomliC - comment - 5 Jun 2016

@wojsmol Please, review all your files changed before ;-
Still some issues (and not have yet check all...)

Another question: is this PR supposed to be a full com_content code style review ?

avatar joomla-cms-bot
joomla-cms-bot - comment - 6 Jun 2016

This PR has received new commits.

CC: @andrepereiradasilva


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 6 Jun 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 6 Jun 2016

I have tested this item successfully on f8c3154


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

avatar joomla-cms-bot
joomla-cms-bot - comment - 16 Jun 2016

This PR has received new commits.

CC: @andrepereiradasilva


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 16 Jun 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 16 Jun 2016

I have tested this item successfully on d1796dc


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

avatar wojsmol
wojsmol - comment - 16 Jun 2016

@JoomliC I reviewd my chenges one more time.

avatar brianteeman
brianteeman - comment - 2 Aug 2016

@zero-24 @JoomliC Can you test this please and then we can get it merged


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

avatar JoomliC
JoomliC - comment - 2 Aug 2016

@brianteeman For changes done, it's almost good for me. (added 2 notes)

Additionnal comments :

  • PR has conflicts
  • I didn't have checked not displayed lines if missing code review
  • views files not reviewed (so not a complete component CS review) but this would need before to operate update of Travis, so just a mention, not a PR blocker.

I would say: fix branch conflicts and at least 1 note for label/desc order in xml and it's ok for me! ;-)

avatar joomla-cms-bot joomla-cms-bot - change - 2 Aug 2016
Category Code style Repository Unit Tests Administration Components SQL Postgresql MS SQL Code style
avatar joomla-cms-bot joomla-cms-bot - change - 2 Aug 2016
Labels Added: ?
avatar brianteeman
brianteeman - comment - 2 Aug 2016

That rebase isnt correct unless you really did edit 800+ files ;)

avatar wojsmol
wojsmol - comment - 2 Aug 2016

@brianteeman last good commit is d1796dc

avatar wojsmol
wojsmol - comment - 3 Aug 2016

@JoomliC @andrepereiradasilva Plese do a final check.
@brianteeman Please remove Unit/System Tests label.

avatar joomla-cms-bot joomla-cms-bot - change - 3 Aug 2016
Category Code style Repository Unit Tests Administration Components SQL Postgresql MS SQL Administration Components Front End Code style
avatar brianteeman brianteeman - change - 3 Aug 2016
Labels Removed: ?
avatar brianteeman
brianteeman - comment - 3 Aug 2016

I've removed the label

avatar JoomliC
JoomliC - comment - 7 Aug 2016

@wojsmol i will redo a full review tomorrow ;-)

avatar wojsmol
wojsmol - comment - 8 Aug 2016

@JoomliC @andrepereiradasilva This branch contains conflicts again, check it out after work.

avatar joomla-cms-bot joomla-cms-bot - change - 8 Aug 2016
Category Code style Administration Components Front End Repository Administration Components Language & Strings Templates (admin) Front End Code style
avatar joomla-cms-bot joomla-cms-bot - change - 8 Aug 2016
Labels Added: ?
avatar JoomliC
JoomliC - comment - 9 Aug 2016

@wojmol did a full review of changes, and it's ok for me!
Just the 3 mentionned by note lines to be removed ;-)


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

avatar wojsmol
wojsmol - comment - 9 Aug 2016

@JoomliC conflict markers removed in 289975c
@brianteeman Pleaseremove Language Change label - added by @joomla-cms-bot as a effrct of bad rebase.

avatar JoomliC JoomliC - test_item - 9 Aug 2016 - Tested successfully
avatar JoomliC
JoomliC - comment - 9 Aug 2016

I have tested this item successfully on 289975c

Code review + Patch applied
All seems good!

?


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

avatar wojsmol
wojsmol - comment - 14 Aug 2016

Small rebase after #11483

avatar wojsmol
wojsmol - comment - 11 Sep 2016

rebase after recent changes

avatar Quy
Quy - comment - 24 May 2017

Is this outdated?

avatar wojsmol
wojsmol - comment - 24 May 2017

@Quy Tgis is very oudated especially after recent PR's from @brianteeman.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 May 2017

@wojsmol should this PR then closed?

avatar wojsmol wojsmol - change - 24 May 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-05-24 15:48:10
Closed_By wojsmol
avatar wojsmol wojsmol - close - 24 May 2017
avatar joomla-cms-bot joomla-cms-bot - change - 24 May 2017
Category Code style Administration Components Front End Repository Language & Strings Templates (admin) Administration com_content Front End Code style Components
avatar wojsmol
wojsmol - comment - 24 May 2017

When I have several free hours, I will go over com_content again and create a new PR if needed.

Add a Comment

Login with GitHub to post a comment