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 <[email protected] > <mailto:[email protected]>> 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 <[email protected] > <mailto:[email protected]>> 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 > [email protected] <mailto:[email protected]> > http://lists.roundcube.net/mailman/listinfo/dev > > > > _______________________________________________ > Roundcube Development discussion mailing list > [email protected] > http://lists.roundcube.net/mailman/listinfo/dev _______________________________________________ Roundcube Development discussion mailing list [email protected] http://lists.roundcube.net/mailman/listinfo/dev
