Menu
▾
▴
Re: [Tiki-devel] Code reformat for Tiki 18
|
From: Jean-Marc L. <jea...@gm...> - 2017-09-21 23:45:24
|
Hi Ricardo,
Interesting topic.
I have some concerns and comments.
First, -1 for spaces instead of tabs. If it's not possible to use Zend
standard with this exception, I'm in favor of choosing another standard
with tabs. Reasons have been discussed already, let's keep this short :-)
Secondly, I see a lot of focus about tools, but I remember we once had very
powerful tools. It was a jenkins VM running a continuous integration syntax
testing with all the syntax rules adapted to our code, plus a website for
looking up any syntax errors.
It was disabled "in order to lessen the load on the host server" because
the single person who looked at the results and fixed the errors and
committed the fixes went away from the community and nobody was interested
in the extra work. It's still on one server.
Also, there was a lot of hope of making Tiki better by making the code
better, but it's not that obviously linked. You can have beautiful code
which does things in wways users don't understand. Sometimes you even have
suggestions of removing features so that the code is better. Isn't the code
supposed to make things possible?
In the same way, I once went through the whole Tiki code to fix all mixed
tabs+spaces files (manually, so as to not risk regressions) but after a
while, the spaces and random indentations come back. It's hard to find an
opportunity window for such a task (no merge period, etc) and again, it's
mainly a manpower issue.
+1 for no mixing of tabs and spaces and finding ways so the spaces don't
creep in again.
Whatever we decide, scary stuff is reserved for the version after an LTS.
That's where the big breakages usually happen, then we have 2 versions of
trying out and fixing and then converging into a stable secure LTS. I
realise this does not sound as disruptive as Bootstrap or the tracker
revamp or the permissions revamp, but it's still going to disrupt a lot of
svn history track, which won't help figuring out if a bug was due to the
reformatting or if it happened before…
So, -1 for Tiki18.
About Luci's question about Smarty code mixed with HTML code or PHP code
mixed with HTML:
I have been through all templates for reformatting and my conclusion is:
there is no rule which will apply everywhere. You think your example is
complex? We have much worse in places where we have conditional {capture}
html bits which totally break the html indenting.
Not because of anyone's fault, just because some smarty templates do really
elaborate things (which is good!).
My view is, the only purpose of indenting is to help readability (the php
compiler doesn't care), so when in doubt, our priority is readability and
we acept that it's not going to be perfect.
Cheers,
J-M
On Mon, Sep 18, 2017 at 11:30 AM, luciash <lu...@ti...> wrote:
> Hi Ricardo,
>
> I accepted your commit message to svn mailing list (it was over the 200 KB
> size limit - around 300 KB) for the cs-fix experimental branch. More inline:
>
> On 18.9.2017 11:04, Ricardo Melo wrote:
>
> Hi Victor,
>
> Thank you for your questions, sorry if I was not explicit enough in my
> email, my suggestion has 2 components:
>
> - Align with one of the existing coding standards ( I suggest to keep
> using Zend Framework, that is aligned with PSR-2, and all them use spaces )
> - Reformat all code to align with that standard
>
> So, regarding your questions:
>
> 1. Yes, I reformatted all code to align with Zend Framework Coding
> standards, and they use spaces - I'm not implying that that will be the
> decision, but as I mention is my suggestion.
>
>
> -1 to use spaces instead of tabs (as discussed many times in Tiki
> community already)
>
> Otherwise +1 for some code reformatting to get more consistent code when
> we are out of auto-merging but it is still not clear to me how to indent
> mixed code. E.g. Smarty code mixed with HTML code or PHP code mixed with
> HTML?
>
> Example:
>
> {* Foo Bar *}
> <div class="foo">
> {if $bar}
> <div class="bar">
> <h2>Bar</h2>
> </div>
> {else}
> <h2>Foo</h2>
> {/if}
> </div>
>
> or:
>
> {* Foo Bar *}
> <div class="foo">
> {if $bar}
> <div class="bar">
> <h2>Bar</h2>
> </div>
> {else}
> <h2>Foo</h2>
> {/if}
> </div>
>
> I am not sure which one is better because while the first example has its
> own indentation levels kept for Smarty and HTML independently it is less
> readable for coders but the second one has wrong indentation in rendered
> HTML output then.
>
> luci
>
>
>
>
> 2. Yes is scary :-) I've done a good amount of visual checking - and some
> tests with profile loading - before pushing the code to SVN and it looks to
> be doing the right decisions. It possible that something will break, and
> that's also a good reason to do before branching for Tiki18.
>
> Thank you Victor,
> Ricardo
>
>
>
> On Mon, Sep 18, 2017 at 7:48 AM, Victor Emanouilov <ti...@em...>
> wrote:
>
>> Hi Ricardo,
>>
>> I think this is a nice try but have 2 concerns:
>>
>> 1. I see experimental branch you mention is using spaces for indentation
>> and I don't see an agreement we switch to spaces from tabs.
>>
>> 2. Did you or is anyone willing to manually go through all those changes
>> (visually) to ensure code is consistent? Otherwise, we depend on an
>> automated tool and pretty limited set of automated tests to ensure code
>> behaves the same as before which looks a bit scary to me.
>>
>> Regards,
>> Victor
>>
>> On 09/17/2017 11:23 PM, Ricardo Melo wrote:
>>
>> Hi Devs,
>>
>> Today, in tiki there is a wide (and inconsistent) range of code format
>> styles.
>>
>> Some effort was done, for instance to have an configuration for phpstorm
>> allowing everyone to use the same standard, but depending on the file that
>> we work, the format might differ and there is a significative number of
>> places with mix of space and tabs in the same file (as an example).
>>
>> I would like to propose a cleanup after the end of semi-automated merges
>> and before the release of Tiki 18.
>>
>> The negative impact of this changes, as I see it, is that backport will
>> be harder, if there are fixes that need to be ported from trunk to 17.x or
>> 15.x, but on the other side, that normally is expected to be "small" things.
>>
>> On the other side, the advantages (from my point of view) are huge -
>> apart from having some consistency - specially if we want to add any kind
>> of automation around code quality and make a bit more easy (and consistent)
>> to have new developers ramp up on tiki. And I think the time to modernise
>> the code is now, before Tiki 18.
>>
>> At the end of the email there is a list of links (from my research) if
>> you want to have a look on how the PHP community as well as both Zend and
>> Symfony are tackling this issue.
>>
>> Currently from Tiki: https://dev.tiki.org/DevTips#PHP_coding_habits
>>
>> - Coding standards: please use Zend Framework Coding Standard for
>> your code.
>> - Exceptions WE USE TABS
>> - Indentation using tabs rather than spaces
>> - The Zend coding standard was adopted after Tiki had accumulated
>> a significant PHP code base which did not follow this standard. Tiki’s
>> current PHP code is still far from adhering to this standard everywhere, in
>> particular when it comes to function names, which were previously written
>> completely lowercase, with underscores between words. For example, as of
>> 2017-01-22, we have clean_logs() , which should be named “cleanLogs”
>> according to our standards. Some old sets of functions called dynamically
>> even require to be named fully lowercase. For example, the FOO plugin
>> requires a function named “wikiplugin_foo” as of 2017-01-22.
>> - Use brackets in your control structures: “if {[...]} else {[...]}”
>> (even if there is only one instruction)
>> - All the strings are written to be easily translated using the tra()
>> function. E.g. tra(‘some string’)
>> - Do not end a php file with ?>
>>
>>
>> Regarding standards, I suggest that we keep as a standard the same as
>> Zend Framework, that either way have evolved and are now aligned with PSR2
>> + some checks ( https://github.com/zendframewo
>> rk/zend-coding-standard/blob/master/ruleset.xml ).
>>
>> After reformatting the code we will still have gaps like the ones
>> mentioned above (function names, code and symbol definition in the same
>> file), but we will be able to handle/refactor that over time as long as we
>> keep a strict standards for all new code being build for Tiki - I've
>> started working together with Marc Laporte on having some automated checks
>> around code quality and unit tests that will help/facilitate the work of
>> keeping the standards to that level.
>>
>> One of the key points is that needs to be super easy to format the code,
>> and as an exercise I've used the same tools used by Zend Framework (
>> https://github.com/squizlabs/PHP_CodeSniffer) to check / reformat the
>> code and committed for your comments / review a copy of trunk reformatted
>> to an experimental branch: https://sourceforge.net/p/tiki
>> wiki/code/HEAD/tree/branches/experimental/cs-fix/
>>
>> I've relaxed some conditions (line length, and check for code and class
>> in the same file) to keep the focus on the essential problems, the output
>> of running the code checker after the reformat is here (
>> https://drive.google.com/open?id=0B2kXdGAyBcseUlVkT0xnRUt6aGc ) if you
>> want to have a look.
>>
>>
>> As a recap:
>>
>> - I would like to clean the code for Tiki and apply a consistent
>> standard;
>> - I think that standard should keep being Zend Framework (is aligned
>> with PSR-2) and we can use the same tools / settings as Zend Framework.
>> - If you want test one version of Tiki reformatted, is available in
>> the experimental branch: https://sourceforge.net/p/tiki
>> wiki/code/HEAD/tree/branches/experimental/cs-fix/
>> <https://sourceforge.net/p/tikiwiki/code/HEAD/tree/branches/experimental/cs-fix/>
>> ;
>> - If you have any concern, comment, suggestion, please use this email
>> thread. I hope we can clear any concern and reformat the code in trunk as
>> soon as we stop semi-automatic merging;
>>
>>
>> Thank you all, looking forward for your comments
>>
>> Also thanks to Marc Laporte for supporting this initiative.
>>
>> Cheers,
>> Ricardo
>>
>> Reference information:
>>
>> - PSR
>> - http://www.php-fig.org/psr/psr-1/ - Basic Coding Standard
>> - Since one of the key points is: Files SHOULD either declare
>> symbols (classes, functions, constants, etc.) or cause side-effects (e.g.
>> generate output, change .ini settings, etc.) but SHOULD NOT do both
>> - So adherence to PSR-0 / PSR-4 is almost implicit
>> - http://www.php-fig.org/psr/psr-0/ - Autoloading Standard
>> - http://www.php-fig.org/psr/psr-4/ - Autoloader
>> - http://www.php-fig.org/psr/psr-2/ - Coding Style Guide
>> - Zend Coding Standards
>> - https://github.com/zendframework/zend-coding-standard - used by
>> all the different Zend Framework packages
>> - https://github.com/zendframework/zendframework/wiki/Coding-
>> Standards (text version slightly outdated comparing with the
>> coding standard code)
>> - Symfony Coding Standards
>> - http://symfony.com/doc/current/contributing/code/standards.html
>>
>>
>>
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>
>>
>>
>> _______________________________________________
>> TikiWiki-devel mailing lis...@li...https://lists.sourceforge.net/lists/listinfo/tikiwiki-devel
>>
>>
>>
>> ------------------------------------------------------------
>> ------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> TikiWiki-devel mailing list
>> Tik...@li...
>> https://lists.sourceforge.net/lists/listinfo/tikiwiki-devel
>>
>>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>
>
>
> _______________________________________________
> TikiWiki-devel mailing lis...@li...https://lists.sourceforge.net/lists/listinfo/tikiwiki-devel
>
>
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> TikiWiki-devel mailing list
> Tik...@li...
> https://lists.sourceforge.net/lists/listinfo/tikiwiki-devel
>
>
|