? Pending

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
31 Dec 2016

Inspired by the changes @andrepereiradasilva did in #13427

Summary of Changes

  • unify language ini file parsing by introducing a JLanguageHelper::parseIniFile($fileName) method.
  • remove unnecessary str_replace call (which is also producing unnecessary ""), because the QQ is automatically taken care of the defined QQ constant. Didn't detect any issues by removing it. Please correct me if I am wrong here.
  • Some minor variable name fixes that were in the scope of code changes that I did

Testing Instructions

  1. Code review
  2. a) Test normal "_qq_" string translation in site and administrator
    b) Test introducing overrides with "_qq_" string translation in site and administrator

Documentation Changes Required

No, if fully automated generation

avatar frankmayer frankmayer - open - 31 Dec 2016
avatar frankmayer frankmayer - change - 31 Dec 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 31 Dec 2016
Category Administration com_languages Libraries
avatar frankmayer frankmayer - change - 31 Dec 2016
Labels Added: ?
avatar frankmayer
frankmayer - comment - 31 Dec 2016

[EDIT] Could we please change the CS rule that expects exactly 2 spaces in Multi-line function declaration? And also maybe to allow spaces to indent in these cases?
It is easier to read the way I did it in this PR. (Not the one with the spaces instead of the tabs of course. Will change that into tabs, too)

FOUND 4 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 185 | ERROR | Multi-line function declaration not indented correctly; expected
     |       | 2 spaces but found 46
 185 | ERROR | Tabs must be used to indent lines; spaces are not allowed
 332 | ERROR | Multi-line function declaration not indented correctly; expected
     |       | 2 spaces but found 14
 332 | ERROR | Tabs must be used to indent lines; spaces are not allowed
--------------------------------------------------------------------------------

avatar mbabker
mbabker - comment - 31 Dec 2016

That's a subjective debate you'll never win. If you really wanna try to get a change approved though, go for something like what PSR-2 suggests (essentially the same way a multi-line array is handled):

screen shot 2016-12-31 at 10 05 50 am

avatar frankmayer
frankmayer - comment - 31 Dec 2016

@mbabker You're (most) probably right. I'll also conform to the current standards for this PR, but I will open an issue for that, based on your suggestion. Thanks.

avatar frankmayer
frankmayer - comment - 31 Dec 2016

CS issues fixed. Ready for reviews & tests!

avatar jeckodevelopment
jeckodevelopment - comment - 2 Jan 2017

@frankmayer can you please look at the conflicting file?

avatar andrepereiradasilva
andrepereiradasilva - comment - 2 Jan 2017

@frankmayer i was actully doing this as part of the _QQ_ deprecation :)
I was already in the testing phase of the PR, but i got sick

Anyway since you already done it, please compare with the PR in my repo https://github.com/andrepereiradasilva/joomla-cms/pull/121/files if you want

avatar frankmayer
frankmayer - comment - 2 Jan 2017

@andrepereiradasilva Oops!!! I wasn't aware that you were also doing unification... We had the same thing in mind ?
So, please do your PR as planned. I'll close mine instead.
And please take anything you might need from this one.

PS. I hope you're better now.

avatar frankmayer frankmayer - change - 2 Jan 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-01-02 15:52:47
Closed_By frankmayer
avatar frankmayer frankmayer - close - 2 Jan 2017
avatar andrepereiradasilva
andrepereiradasilva - comment - 2 Jan 2017

there was no need to close it. but ok i will submit mine when tested

Add a Comment

Login with GitHub to post a comment