[RCD] Codebase cleanup, PSR-1/2 compliance - and before you say no, I'm offering to do the work

David Deutsch skoremail at gmail.com
Thu Aug 29 11:31:32 CEST 2013

Hey guys,

I went ahead with making the plan and put up a gist here:

Let me know what you think!

I would estimate that this will take a week or two at most. The larger
blocks will be one or two PRs per day, smaller ones maybe 6 or 7. Didn't
find a good way to tackle doing lesser updated directories first, but I
think the splitting up of the PRs will help reduce the pain of merge
conflicts. Although, of course, that will also depend a tiny bit on how
fast you merge ;-)


On Sun, Aug 25, 2013 at 7:39 PM, David Deutsch <skoremail at 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 at 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 at lists.roundcube.net
>> http://lists.roundcube.net/mailman/listinfo/dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.roundcube.net/pipermail/dev/attachments/20130829/21f92a61/attachment.html>

More information about the dev mailing list