Last link should be: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_logger/runlog/runlog.php#L242
On Fri, Aug 30, 2013 at 10:57 AM, David Deutsch <[email protected]> wrote: > 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 <[email protected]>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 >> <[email protected]>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 >>> [email protected] >>> http://lists.roundcube.net/mailman/listinfo/dev >>> >> >> >
_______________________________________________ Roundcube Development discussion mailing list [email protected] http://lists.roundcube.net/mailman/listinfo/dev
