[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
Sun Aug 25 19:39:44 CEST 2013


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/20130825/d5d7baab/attachment.html>


More information about the dev mailing list