Alright, commit for the four spaces in the CSS files was added to the pull request.

Added a second PR that tackles four further plugins: https://github.com/roundcube/roundcubemail/pull/110

So far, a couple of things came up in the discussion of the first PR:

Biggest concern was use of braces and one-line statements. As I had written earlier, I have two major rules that are in competition - First one is to write as "properly" as possible, meaning braces go basically everywhere. Second one is that if the situation allows for it, expressions should take up as few lines as possible while staying below the 80 character limit. Braces can be visual clutter here.

That means we have a general conflict between "keep it as simple as possible" and "keep it as concise as possible". As a first example, this is where I join two lines into one:

https://github.com/daviddeutsch/roundcubemail/commit/67426ea779c7c6d224b3947559dd9ae37b29b437
https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b9478ccfa3491d34053d7

I.E., in the first link, the $cache variable is used precisely one more time and I don't think joining it into a one-liner makes the code less readable (I would argue it makes it more readable, since everything is in one place).

As a counter-example, here, the expression rcmail::get_instance() is called in six lines, making things overly complicated. Here, I declared one $rcmail variable in the beginning and use that from there on. It actually also helps reduce the line numbers later on:

https://github.com/daviddeutsch/roundcubemail/commit/92666405bcc2068a52bb32ab4ed7220f55ae7e33

Getting to the more controversial stuff: I really like getting rid of indents and long braced statements. Here is a small example:

https://github.com/daviddeutsch/roundcubemail/commit/fee02cb397620922b94336a315b1089c6bef292d

Everything is stuffed into one if statement. Instead, you can just do the if statement and return; in case it isn't true. It's not as obvious here, but this comes in really handy when you have longer statements. So here is a medium sized example:

https://github.com/daviddeutsch/roundcubemail/commit/8ef53237ec2618b759cfc3ee5becd8ea12fd72e9#L2L214

The original ends up at four indents while my cleanup sits at one. (Well, one and a half since I'm kinda-sorta cheating with the if one-liners.) The control flow is a lot simpler to follow here:

- Do we have print_to_console active? If not, return.

- Is it not an array? If yes, do something, then return.

- Otherwise check for a tag in the array - if yes, do something.

I end up with two more lines total, but I think the code is much, much more readable:

Original: https://github.com/roundcube/roundcubemail/blob/master/plugins/debug_logger/runlog/runlog.php#L174

Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_logger/runlog/runlog.php#L268

Things like this one will become even more apparent when I tackle longer statements.

cheers,
David

On Thu, Aug 29, 2013 at 5:50 PM, David Deutsch <skoremail@gmail.com> wrote:

> Hmm, we have 4 spaces everywhere else (and in Larry skin we have tabs I suppose). I'd either go for tabs or 4 spaces as well.

Agreed, I'll switch it to four spaces.

> One more thing: I can already see ourselves in merging hell when we want to merge other pull requests or development branches back into master because most likely a merge attempt will result in conflicts.

Yeah, that was my main concern as well. I've checked through the current PRs and it seems like none of them touch the plugins. Since I'm working on those first, that's fine - you should have a bit of time to merge those in until I'm done. If there are PRs that cannot be resolved quickly, we might have to check on them on a case-by-case basis. If it's just a couple of files, I can always roll back changes and redo them, although obviously I'd like to avoid that, if possible. As for branches - I only saw the release branches. Do you mean local dev branches?

cheers,
David

On Thu, Aug 29, 2013 at 5:40 PM, Thomas Bruederli <thomas@roundcube.net> wrote:

David Deutsch wrote:
> First pull request is up: https://github.com/roundcube/roundcubemail/pull/109

Thanks! We'll read through it.

>
> There were a couple of things that might turn out to be controversial, I
> think mostly with calling on the html class - those seem to have a tendency
> to get out of hand, so I decided to split it over multiple lines.
>
> For CSS, I went with a two spaces indent, which seems to be the most
> commonly used in the codebase. Let me know if you want four spaces instead.

Hmm, we have 4 spaces everywhere else (and in Larry skin we have tabs I
suppose). I'd either go for tabs or 4 spaces as well.

One more thing: I can already see ourselves in merging hell when we want to
merge other pull requests or development branches back into master because
most likely a merge attempt will result in conflicts. Any suggestions how
that could go smooth? Maybe we should first clean up all of our pending PRs
and try to eliminate existing dev branches before the cleanup really gets
started.

Kind regards,
Thomas

_______________________________________________
Roundcube Development discussion mailing list
dev@lists.roundcube.net
http://lists.roundcube.net/mailman/listinfo/dev