David Deutsch wrote:
Hey guys,
Hello David
I went ahead with making the plan and put up a gist here: https://gist.github.com/daviddeutsch/6376013
Thanks for this! Here are some comments about your proposal:
- PSR-2 compliance
I generally agree to using PSR-2 even if there are some rules that personally I don't like. Especially the if/else newlines.
Spaces after parenthesis is definitely a no-go!
- I will ruthlessly convert instances of yoda conditions.
I veto against this as we (almost) consequentially write condition statements in yoda style. Ain't broken but will highly confuse the core developers who work with the codebase every day.
- /program/localization - mostly just indents
You should not touch the /program/localization/*/*.inc files as they're automatically generated by Transifex but I agree to modify index.inc.
- PEAR vs. composer
Roundcube still supports PHP 5.2 and thus Composer is not (yet) an option. Just ignore that for the moment. Be assured that as soon as we drop support for PHP < 5.3 we'll fully switch to Composer.
- /installer
The installer generally needs a refactoring with proper MVC separation and localized texts. Feel free to dig into this.
- Skins
Yes, the images are run through pngcrush. Actually I'm using http://imageoptim.com which uses different optimization algorithms.
The templating system of Roundcube was created before the all the popular template engines raised. Switching to twig or handlebars would be nice but requires proper planning and a sustainable path to keep backwards compatibility with all the existing skins out there. Please just ignore that for now.
LESS would definitely be an option for CSS. Unfortunately none of as experience with it and no time to dig into that (I admit being one of the very old school web developers from the last century). For releases I'd prefer to compile the less files and deliver static CSS files. That however adds node to the dependency chain of Roundcube.
I have to mention that personally I don't force such refactorings as they make it more difficult to track (functional) changes in the SCM. But I can see the overall benefit and I very much appreciate your efforts to contribute to this.
Best regards, Thomas
On Sun, Aug 25, 2013 at 7:39 PM, David Deutsch <skoremail@gmail.com mailto:skoremail@gmail.com> wrote:
Hey, it seems like we're halfway there to you guys agreeing on this. Sweet! ;-) > all of us (read: I and Thomas ;)) prefer no spaces here Sure, no problem. > However, I saw Thomas uses the later. Well, since I and PSR-2 agree with Thomas here, maybe it's a good idea to go with that? ;-) > I don't like for/foreach in few lines I think you're talking about the RedBeanPHP stuff I linked, right? (Couldn't find any instance in the Roundcube examples I sent you, but I'm tired, so I might have missed something...) In that case, it was accepted policy in that code, so I went with it. I generally like it in some instances - again, when it helps understanding the code and if it serves staying under the 80 character limit. I was originally hesitant myself, but nowadays, I quite like it, it saves up on generated variables, too. Well, maybe this is something for later ;-) > No. As you might notice we are using branches and git-master is in (active) development mostly all the time Yes, I did see that, which is why I posted the question - I just wasn't sure whether it is "very" active right now since I have no frame of reference. > I don't know, maybe we shouldn't do such changes before 1.0 release. Quite honestly, that is a recipe for "never" ;-) Not saying that there will never be a 1.0 release, but that pushing codebase cleanup usually begets further pushing of codebase cleanups. It is your choice, of course, all I can do is offer my help. But I actually think a good cleanup before a 1.0 is a very good thing since you'll likely be maintaining 1.0 code for quite a while (legacy setups etc.), so having that clean and syntactically close to the repository is usually a good way to avoid confusion. Maybe we can do it like this: I would set a schedule of how I would iterate through the codebase and make individual pull requests for major directory blocks (say a couple hundred or low-thousand changes per PR). I would try to set it up in such a way that I touch the most frequently touched areas last. That way, you can already test drive my cleanup in low-interest areas and aren't interrupted as much by me meddling with stuff you're often working on. Of course, I would put up the list here first, so you can veto or change it, since I very probably will miss some obvious cues. In general, the process will be two- or three-passed. First pass is just basic formatting (largely automatic), second pass is in-depth stuff like proper PHPdoc blocks (partly automatic), third pass is actually hunting for bugs or clearing up structures that could be unclear. So the idea with that is that I could roll back to a less controversial commit within a PR and we could already commit that while discussing the stuff that didn't turn out as acceptable. cheers, David On Sun, Aug 25, 2013 at 5:58 PM, A.L.E.C <alec@alec.pl <mailto:alec@alec.pl>> wrote: On 08/25/2013 02:58 PM, David Deutsch wrote: > > Space after/before brackets > > But again, this is up to taste and it's a setting in an automatic > formatter, anyhow, so while I have a taste in it, I don't really have > strong feelings either way. Yes. We are consistent in this through our code base, which means all of as (read: I and Thomas ;)) prefer no spaces here. So, we'd like to keep this and your commits will be much smaller. > New line before else/catch > > I suppose you mean between brace and statement? PSR-2 is rather specific on > no newline between brace and statement and I must say I agree with that. If > an if/else is hard to read, it's usually the content between the brace and > fanning out the braced is just a bandaid. It's mostly a problem with long > if/else statements, for instance: I just find better to read: } else if (true) { something(); } else { something2(); } than } else if (true) { something(); } else { someting2(); } However, I saw Thomas uses the later. > Is there anything else that you find needs discussion or was the rest OK? I don't like for/foreach in few lines, as I saw somewhere in your links: foreach( array( elem1, elem2, elem3,... ) as $var ) { > Another note - since large codebase changes like this one can be a problem > in terms of getting clean merges during active work in other areas, it is > usually preferable to do them when there isn't that much activity. Since > 0.9.3 was just released - would at the moment be a good time? No. As you might notice we are using branches and git-master is in (active) development mostly all the time. I don't know, maybe we shouldn't do such changes before 1.0 release. -- Aleksander 'A.L.E.C' Machniak LAN Management System Developer [http://lms.org.pl] Roundcube Webmail Developer [http://roundcube.net] --------------------------------------------------- PGP: 19359DC1 @@ GG: 2275252 @@ WWW: http://alec.pl _______________________________________________ Roundcube Development discussion mailing list dev@lists.roundcube.net <mailto:dev@lists.roundcube.net> http://lists.roundcube.net/mailman/listinfo/dev
Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/mailman/listinfo/dev