A.L.E.C > This is not so exciting for some of us ;)

Yes, I understand that. Gabor from RedBeanPHP (the other thing I cleaned up recently) is also very hesitant about namespaces, actually published a thing on his roadmap saying it would never happen and linking to a popular article that I now can't find since he removed it after I persuaded him. Well, not persuaded him that namespaces in PHP are super great, just that they're the best we can do for now, so we might as well roll with them ;-)

A.L.E.C > If you will install it a lot you'll most likely do not use installer at all.

True, but I haven't given all that much of a shot yet, so I don't know what will end up... catching my fancy.

Cor Bosman > combination of composer, blade (template engine), less, and automated css/js creation is just so very smooth. Would love for RCM to head the same way.

Absolutely agree. Things are moving quite rapidly in PHP at the moment, particularly driven by composer. Would love to be on board and chip in the knowledge that I have if/when RC makes that switch.

And yes, particularly asset management is one of the fields where you start not minding it that much, then you start working out a number of approaches and then you just, over the years, accumulate lots and lots of technological debt. Having a good way to just write assets very conveniently and then utilize a build or setup process to manage everything properly is absolute bliss when you're used to hand-coding everything until you want to pull out your hairs so you can finally do something that more pleasant than manual asset management.

Not sure how RCM will get there eventually, but I'm a big fan of taking the concept of semantic versioning as an invitation, where major version jumps are those where you are allowed to breaks lots of stuff. Otherwise you just end up in a codebase that spirals downwards because you both don't really can, but also don't really want to change anything too major.

cheers, David

On Thu, Aug 29, 2013 at 2:47 PM, Cor Bosman cor@xs4all.nl wrote:

This all sounds like some very exciting changes. I have been doing a lot of work with Laravel lately, and the combination of composer, blade (template engine), less, and automated css/js creation is just so very smooth. Would love for RC to head the same way.

Cor

---

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

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

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/67426ea779c7c6d224b3947... https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b947...

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/92666405bcc2068a52bb32a...

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/fee02cb397620922b94336a...

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/8ef53237ec2618b759cfc3e...

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/... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_l...

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.netwrote:

...

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

Almost done with the third PR, again tackling 4 plugins (hey, almost halfway through!).

Another good example came up during that. In this commit: https://github.com/daviddeutsch/roundcubemail/commit/7c041eda2d68a59486574d3...

Original: https://github.com/roundcube/roundcubemail/blob/master/plugins/archive/archi... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-3/plugins/archive...

In the original, we have this long if/elseif/elseif statement spanning 40 lines, topped off with a hard to parse starter if. The funny thing is - we're really only concerned with two cases to begin with, if the task is 'settings' or 'mail'. (Here, I'm actually not sure whether setting self::$task to 'mail|settings' earlier means that only those two arrive at the init() function to begin with... would make this even funnier...)

So I make that a condition right at the start - no other code in this needs to be executed if we don't have that met. Next up, I handle the two smaller cases first, to get them out of the way. I also pull $archive_folder into its own line - this code is executed anyways, so doing it within the if statement might be shorter, but also clutters up the statement and makes it harder to understand. in_array() makes the if even shorter and we get a nice, concise if statement for the second case.

Now that these are out of the way, we can simply run through the long if body statement outside of the if, again making the code easier to parse. (Granted, the syntax formatting already makes it easier to understand.)

On Fri, Aug 30, 2013 at 11:30 AM, David Deutsch skoremail@gmail.com wrote:

Last link should be: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_l...

On Fri, Aug 30, 2013 at 10:57 AM, David Deutsch skoremail@gmail.comwrote:

...

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/67426ea779c7c6d224b3947...

https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b947...

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/92666405bcc2068a52bb32a...

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/fee02cb397620922b94336a...

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/8ef53237ec2618b759cfc3e...

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/... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_l...

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.comwrote:

...

...

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.netwrote:

...

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

David Deutsch 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.

We started to add braces everywhere just to make the code consistent it simpler to add debug code in between.

And what's this 80 characters discussion anyways? Who still has a screen that only fits 80 characters? Line breaks should be added where it makes sense but please don't take these 80c as the holy limit.

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/67426ea779c7c6d224b3947... https://github.com/daviddeutsch/roundcubemail/commit/08a50e4cb320bf6db66b947...

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).

Fully agreed. No variables needed if it's only used once (or none).

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/92666405bcc2068a52bb32a...

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/fee02cb397620922b94336a...

Everything is stuffed into one if statement. Instead, you can just do the if statement and return; in case it isn't true.

Single exit point of a function! Don't spread return statements all over.

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/8ef53237ec2618b759cfc3e...

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.

Exception: return allowed at the start of a function if preconditions are not met.

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/... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/cleanup-2/plugins/debug_l...

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

Again: please no return statements in the middle of a function.

~Thomas

Good question! (Also: shame on me for not finding those guidelines ealier O_o .../awkward/)

I think the basic recommendation that PSR-1 makes on this are pretty sound: Make a choice and stick with it. As a personal preference, I use StudlyCaps for classes (this is a 'must' in PSR-1), camelCase for methods and under_scores for variables. I find that that helps separate the three concepts, even though it might seem like "hey, why do you use three ways of doing something". Because it's three separate concepts!

From what I've seen in the plugins so far, it seems like RCM pretty

consistently uses lowercase_underscored for all three.

Sidenote: My IDE actually gives me a lot of funny warnings about the class names in RCM - something along the lines of "there is probably a conceptual error here, somewhere" I always imagine it standing before me with eyes twitching "dude, this aint right!" ;-)

Sidenote 2: The Guidelines references PEAR standards. I used to really dislike those, particularly for the ~80 character line limit, which seemed silly ("Oooh, your b/w terminal can only do 80 characters per line!?"). Fast forward a couple of years and I absolutely love an 80 char line limit. It's like an addiction to shortness.* There are a couple of outdated things in there and I might forever think that 4 spaces is silly, but yeah, not as bad today ;-)

*Sidenote 3: This also helped me come around on Namespaces, really. "use... as" REALLY helps keep lines short when using classes and particularly when calling static methods.

On Fri, Aug 30, 2013 at 2:39 PM, taki taki@alkoholista.hu wrote:

2013-08-29 14:07 időpontban Thomas Bruederli ezt írta:

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.

There's a lots of inconsistency between PSR-1/2 and the current RC Dev_Guidelines (http://trac.roundcube.net/**wiki/Dev_Guidelineshttp://trac.roundcube.net/wiki/Dev_Guidelines) eg. StudlyCaps and camelCase in class and method names. Anyone wants to clarify the current rules? I want to create a ruleset for PHP_CodeSniffer.

-- Sandor Takacs

______________________________**_________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/**mailman/listinfo/devhttp://lists.roundcube.net/mailman/listinfo/dev

PR #5 was submitted earlier: https://github.com/roundcube/roundcubemail/pull/114

Working on managesieve right now and just wanted to quickly share my find of an 10 level indent:

https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l...

This is the first time that I'm considering breaking something up into separate methods. There's a 600 line if/elseif construct in there. I mean come /on/.

-David

On Fri, Aug 30, 2013 at 7:14 PM, David Deutsch skoremail@gmail.com wrote:

PR #4 is in: https://github.com/roundcube/roundcubemail/pull/113

Also note that I'm keeping the gist updated: https://gist.github.com/daviddeutsch/6376013

...with PR links and information that was discussed. Might be useful if somebody wants to write an updated Code Guideline later on.

On Fri, Aug 30, 2013 at 3:02 PM, David Deutsch skoremail@gmail.comwrote:

...

Good question! (Also: shame on me for not finding those guidelines ealier O_o .../awkward/)

I think the basic recommendation that PSR-1 makes on this are pretty sound: Make a choice and stick with it. As a personal preference, I use StudlyCaps for classes (this is a 'must' in PSR-1), camelCase for methods and under_scores for variables. I find that that helps separate the three concepts, even though it might seem like "hey, why do you use three ways of doing something". Because it's three separate concepts!

From what I've seen in the plugins so far, it seems like RCM pretty consistently uses lowercase_underscored for all three.

Sidenote: My IDE actually gives me a lot of funny warnings about the class names in RCM - something along the lines of "there is probably a conceptual error here, somewhere" I always imagine it standing before me with eyes twitching "dude, this aint right!" ;-)

Sidenote 2: The Guidelines references PEAR standards. I used to really dislike those, particularly for the ~80 character line limit, which seemed silly ("Oooh, your b/w terminal can only do 80 characters per line!?"). Fast forward a couple of years and I absolutely love an 80 char line limit. It's like an addiction to shortness.* There are a couple of outdated things in there and I might forever think that 4 spaces is silly, but yeah, not as bad today ;-)

*Sidenote 3: This also helped me come around on Namespaces, really. "use... as" REALLY helps keep lines short when using classes and particularly when calling static methods.

On Fri, Aug 30, 2013 at 2:39 PM, taki taki@alkoholista.hu wrote:

...

2013-08-29 14:07 időpontban Thomas Bruederli ezt írta:

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.

There's a lots of inconsistency between PSR-1/2 and the current RC Dev_Guidelines (http://trac.roundcube.net/**wiki/Dev_Guidelineshttp://trac.roundcube.net/wiki/Dev_Guidelines) eg. StudlyCaps and camelCase in class and method names. Anyone wants to clarify the current rules? I want to create a ruleset for PHP_CodeSniffer.

-- Sandor Takacs

______________________________**_________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/**mailman/listinfo/devhttp://lists.roundcube.net/mailman/listinfo/dev

On Aug 31, 2013, at 2:00 PM, David Deutsch skoremail@gmail.com wrote:

Another possibly neat comparison:

Original: https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/37167c5ce1c00cb4f42b7f59a...

The code may be /slightly/ less DRY, but, I find, a lot more readable.

Definitely more readable.

I must also say that so far, I find very few yoda conditions! ;-)

Find them you will.

Cor

PR #7 is in and pretty much done: https://github.com/roundcube/roundcubemail/pull/116

while PR #6 is still being worked on: https://github.com/roundcube/roundcubemail/pull/115

This is one of the larger ones, so it is expected to take longer. I think I did manage to get some stuff simplified already (see earlier message), but I'm still not that satisfied with the result. I also think that the code itself makes poor use of available stuff in RCM to begin with, like the html class or the ::gettext() method in the plugin, so I will probably look into that. Stuff like that is a lot more draining than regular cleanup, though, so I'll probably keep it on the side and do a little every now and then. It would definitely help to have the permission to add methods to that class since there are a LOT of repeated statements. For now, I will keep a look out for existing methods that I can call in.

PR #7 is a good example for something that Thomas worried about earlier - I didn't see a commit coming in concerning a plugin that I was working on two days ago. I did a local merge of those changes and pushed them upstream to the PR, so everything is fine there. Not sure whether that was understood earlier, but it is of course primarily my responsibility to keep up with commits and make sure a merge is straightforward for you guys.

The next couple of days, I won't have as much time for cleanup, so I would appreciate if we could use that brief timeout to discuss the finer points and have some final decisions going forward. For smaller stuff, I could probably do quick adjustments to follow what was decided - that would help maybe accepting the first PR(s) into the codebase. Not to be too pushy here, of course, just would appreciate seeing actual confirmation that the work I'm putting in really will end up going somewhere ;-)

cheers, David

On Sat, Aug 31, 2013 at 10:53 PM, David Deutsch skoremail@gmail.com wrote:

Thanks.

And find them I did!

It's just that Thomas said earlier they were the standard in the codebase and me changing them would confuse existing developers. So I remain unconvinced there is what I'm saying... ;-)

-David

On Sat, Aug 31, 2013 at 10:43 PM, Cor Bosman cor@xs4all.nl wrote:

...

On Aug 31, 2013, at 2:00 PM, David Deutsch skoremail@gmail.com wrote:

Another possibly neat comparison:

Original: https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l... Cleanup: https://github.com/daviddeutsch/roundcubemail/blob/37167c5ce1c00cb4f42b7f59a...

The code may be /slightly/ less DRY, but, I find, a lot more readable.

Definitely more readable.

I must also say that so far, I find very few yoda conditions! ;-)

Find them you will.

Cor

---

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

And what's this 80 characters discussion anyways? Who still has a screen

that only fits 80 characters? Line breaks should be added where it makes sense but please don't take these 80c as the holy limit.

Yeah, well, as I've said: It's really more of a personal preference of mine. For my own code, I have found that trying to see 80 lines as a semi-holy limit can help to force simplify my approach.

It's obviously no longer about small screens (also my biggest pet peeve about people dismissing tabs as indents because some exotic strawman IDEs can't deal with them properly).

Single exit point of a function! Don't spread return statements all over.

Would need a clarification here. There are tons and tons of examples in the original code where this is not the case. My approach is "Handle corner cases first, then the big pile of code" - so with the examples, get a couple of returns out of the way, then do what the function should usually do.

As an extreme example: You could start a function with an if structure that checks whether all arguments of the function are the correct type. But you wouldn't want that to create a five level indent - you want to handle them in a structure like the way I set them up and then move on to the actual code.

Exception: return allowed at the start of a function if preconditions are

not met.

Yeah, I guess we are in agreement about that, then ;-)

I will go through my changes and see how I would deal with the "returns on top and bottom" restriction.

We made our choice and we're going to stick with it. Thus use underlines

and no caps. And we're not doing PSR-1 here.

Understood.

I'm wondering what your IDE means by "conceptual errors".

Oh I think it just does that with lowercase class names, since that's so uncommon.

We definitely want if ($sky == 'blue') and not if ('blue' == $sky).

Awesome! :-D

Will move on to the GitHub comments now.

Thanks for all the feedback!

-David

On Sun, Sep 1, 2013 at 7:20 PM, Thomas Bruederli thomas@roundcube.netwrote:

David Deutsch wrote:

...

Thanks.

And find them I did!

It's just that Thomas said earlier they were the standard in the codebase and me changing them would confuse existing developers. So I remain unconvinced there is what I'm saying... ;-)

I obviously got your intentions wrong and I'm sorry about this. We definitely want if ($sky == 'blue') and not if ('blue' == $sky).

~Thomas

...

On Sat, Aug 31, 2013 at 10:43 PM, Cor Bosman <cor@xs4all.nl mailto:cor@xs4all.nl> wrote:

On Aug 31, 2013, at 2:00 PM, David Deutsch <skoremail@gmail.com
<mailto:skoremail@gmail.com>> wrote:

...

Another possibly neat comparison:

Original:

https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l...

...

...

Cleanup:

https://github.com/daviddeutsch/roundcubemail/blob/37167c5ce1c00cb4f42b7f59a...

...

...

The code may be /slightly/ less DRY, but, I find, a lot more

readable.

...

Definitely more readable.

...

I must also say that so far, I find very few yoda conditions! ;-)

Find them you will.

Cor


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

---

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

---

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

A proper and predictable process flow is something I learned to

appreciate during my years as a software developer.

Sure, although I don't understand how my approach makes it any less predictable. All I have done is fan out the if/elseif structures and, arguably, that makes it easier to check through them. Maybe changing it to no-oneliner-ifs will make this a little clearer for you, but the return statements really stand out when you scroll through the code - whether or not they are wrapped in if blocks or individually.

What's wrong about if clauses with proper indenting? These can even be

collapsed with a sexy IDE.

Nothing wrong with them. It's just: Do you wrap the entire content of a function in an if or do you make a negated if -> return statement? The first doesn't scale, the second makes things more readable - that was really all I was trying to convey.

If the structure gets too complicated, individual blocks should maybe be

moved to individual functions with sensible names.

Precisely my point ;-) What I'm doing right now is cleanup to make the existing functions easier to read. If adding functions was on the table, my work would be very different, of course.

-David

On Mon, Sep 2, 2013 at 9:13 PM, Thomas Bruederli thomas@roundcube.netwrote:

On Sun, Sep 1, 2013 at 8:08 PM, David Deutsch skoremail@gmail.com wrote:

...

...

And what's this 80 characters discussion anyways? Who still has a screen that only fits 80 characters? Line breaks should be added where it makes sense but please don't take these 80c as the holy limit.

Yeah, well, as I've said: It's really more of a personal preference of

mine.

...

For my own code, I have found that trying to see 80 lines as a semi-holy limit can help to force simplify my approach.

It's obviously no longer about small screens (also my biggest pet peeve about people dismissing tabs as indents because some exotic strawman IDEs can't deal with them properly).

...

Single exit point of a function! Don't spread return statements all

over.

...

Would need a clarification here. There are tons and tons of examples in

the

...

original code where this is not the case. My approach is "Handle corner cases first, then the big pile of code" - so with the examples, get a

couple

...

of returns out of the way, then do what the function should usually do.

I didn't say that our codebase is free of these but I'd not want more intermediate returns to be added but rather getting rid of them. A proper and predictable process flow is something I learned to appreciate during my years as a software developer.

...

As an extreme example: You could start a function with an if structure

that

...

checks whether all arguments of the function are the correct type. But

you

...

wouldn't want that to create a five level indent - you want to handle

them

...

in a structure like the way I set them up and then move on to the actual code.

What's wrong about if clauses with proper indenting? These can even be collapsed with a sexy IDE. If the structure gets too complicated, individual blocks should maybe be moved to individual functions with sensible names.

...

...

Exception: return allowed at the start of a function if preconditions

are

...

...

not met.

Yeah, I guess we are in agreement about that, then ;-)

Great!

...

Will move on to the GitHub comments now.

Aye!

~Thomas

...

On Sun, Sep 1, 2013 at 7:20 PM, Thomas Bruederli thomas@roundcube.net wrote:

...

David Deutsch wrote:

...

Thanks.

And find them I did!

It's just that Thomas said earlier they were the standard in the codebase and me changing them would confuse existing developers. So I remain unconvinced there is what I'm saying... ;-)

I obviously got your intentions wrong and I'm sorry about this. We definitely want if ($sky == 'blue') and not if ('blue' == $sky).

~Thomas

...

On Sat, Aug 31, 2013 at 10:43 PM, Cor Bosman <cor@xs4all.nl mailto:cor@xs4all.nl> wrote:

On Aug 31, 2013, at 2:00 PM, David Deutsch <skoremail@gmail.com
<mailto:skoremail@gmail.com>> wrote:

...

Another possibly neat comparison:

Original:

https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l...

...

...

...

...

Cleanup:

https://github.com/daviddeutsch/roundcubemail/blob/37167c5ce1c00cb4f42b7f59a...

...

...

...

...

The code may be /slightly/ less DRY, but, I find, a lot more

readable.

Definitely more readable.

...

I must also say that so far, I find very few yoda conditions! ;-)

Find them you will.

Cor


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

---

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

---

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

---

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

---

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

"Roundcube, code proudly cleant by argumenting trolls"

On Mon, Sep 2, 2013 at 9:43 PM, Thomas Bruederli thomas@roundcube.netwrote:

On Mon, Sep 2, 2013 at 9:20 PM, David Deutsch skoremail@gmail.com wrote:

...

...

A proper and predictable process flow is something I learned to

appreciate

...

...

during my years as a software developer.

Sure, although I don't understand how my approach makes it any less predictable. All I have done is fan out the if/elseif structures and, arguably, that makes it easier to check through them. Maybe changing it

to

...

no-oneliner-ifs will make this a little clearer for you, but the return statements really stand out when you scroll through the code - whether or not they are wrapped in if blocks or individually.

...

What's wrong about if clauses with proper indenting? These can even be collapsed with a sexy IDE.

Nothing wrong with them. It's just: Do you wrap the entire content of a function in an if or do you make a negated if -> return statement? The

first

...

doesn't scale, the second makes things more readable - that was really

all I

...

was trying to convey.

For that specific "entire content of a function" case I mentioned the exceptions that allow returns at the beginning of a function that check preconditions.

But I'm more referring to your initial refactoring in

https://github.com/daviddeutsch/roundcubemail/blob/cleanup-3/plugins/archive...

In such cases I think a sequence of if/else if/else blocks makes more clear for the reader that these are three distinct cases handled in the plugin init function.

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

if you think people care about a maintainable code base are trolls what do you do on a devel list?

for the ordinary users this may not matter *until* things start to break because nobody konws what the code is suppsoed to doing

Am 02.09.2013 22:03, schrieb stephane martin:

"Roundcube, code proudly cleant by argumenting trolls"

On Mon, Sep 2, 2013 at 9:43 PM, Thomas Bruederli <thomas@roundcube.net mailto:thomas@roundcube.net> wrote:

On Mon, Sep 2, 2013 at 9:20 PM, David Deutsch <skoremail@gmail.com <mailto:skoremail@gmail.com>> wrote:
>> A proper and predictable process flow is something I learned to appreciate
>> during my years as a software developer.
>
> Sure, although I don't understand how my approach makes it any less
> predictable. All I have done is fan out the if/elseif structures and,
> arguably, that makes it easier to check through them. Maybe changing it to
> no-oneliner-ifs will make this a little clearer for you, but the return
> statements really stand out when you scroll through the code - whether or
> not they are wrapped in if blocks or individually.
>
>
>> What's wrong about if clauses with proper indenting? These can even be
>> collapsed with a sexy IDE.
>
> Nothing wrong with them. It's just: Do you wrap the entire content of a
> function in an if or do you make a negated if -> return statement? The first
> doesn't scale, the second makes things more readable - that was really all I
> was trying to convey.

For that specific "entire content of a function" case I mentioned the
exceptions that allow returns at the beginning of a function that
check preconditions.

But I'm more referring to your initial refactoring in
https://github.com/daviddeutsch/roundcubemail/blob/cleanup-3/plugins/archive/archive.php#L17

In such cases I think a sequence of if/else if/else blocks makes more
clear for the reader that these are three distinct cases handled in
the plugin init function.

On Sep 2, 2013, at 4:56 PM, stephane martin stef.martin@gmail.com wrote:

http://en.wikipedia.org/wiki/Humour

Wasn't really funny. We should welcome people that want to work on the code with open arms.

Cor

On 03.09.2013 00:14, David Deutsch wrote:

...

"Roundcube, code proudly cleant by argumenting trolls"

For what it's worth, I found it pretty funny, so: I'll take it! ;-)

I understood it as a comment about all major parties involved in this discussion and not to cause any harm.

I encourage discussion on how to efficiently work on a successful open source project like roundcube. However, there is always the chance of too much policies and regulation instead of common sense and flexibility to know when rules are better bent.

Finally, i want to thank David for his commitment and ability to keep things moving, as well as Alec and Thomas for their openess and coorporation in this regard.

I think, i might not be the only one who feels this way!

Cheers, Raoul PS. No bike-shedding intended.

On 2013-09-03 3:51, David Deutsch wrote:

In this instance, I simply think that the code is easier to read. Just for reference, the difference is between: foreach ((array)$rcmail->config->get('identity_select_headers', array()) as $header) { and

$headers = (array) $rcmail->config->get('identity_select_headers', array())

foreach ($headers as $header) {

I don't have a horse in this race, but just fwiw, I don't find the former difficult to read at all. It _might_ be a code smell, but it wouldn't bother me unless I saw it a lot (i.e., the whole code base was really unnecessarily dense).

When I see that line, I see a "foreach", and I immediately skip to the "as", and as long as that is a sensibly-named variable, then I understand what's going on: "oh, ok, we're iterating over an array of headers, sure." I wouldn't even look very hard at the line noise, unless I was chasing down a bug that might be living in it.

I've been programming for about 25 years -- lots of things in code irritate me, but this isn't one of them.

hth, hand.

• R.

[__ Robert Sheldon [__ No Problem [__ Information technology support and services [__ (530) 575-0278

1. Linebreaks separating variable declaration blocks

Discussion: https://github.com/roundcube/roundcubemail/pull/109#discussion-diff-6064999

I think for this, we will end up reverting some of the changes that I made. But we still have to discuss the details - It's just mainly about the other devs telling me in what instances it would be permissible to separate blocks.

I agree with Thomas that maybe this should happen in two phases. A lot of the time those variable declaration blocks should maybe be done in a totally different way anyways so maybe we should worry about that when we get to that. Maybe when we get to use composer more and can actually re-use composer elements from other projects.

Example.. $headers = rcube_utils::get_input_value('_header', rcube_utils::INPUT_POST); To me this seems overly convoluted. Ive always wondered why there isnt an input class.

$header = Input::post('_header');

Cor

2013-09-03 00:14 időpontban David Deutsch ezt írta:

...

"Roundcube, code proudly cleant by argumenting trolls"

For what it's worth, I found it pretty funny, so: I'll take it! ;-)

...

For that specific "entire content of a function" case I mentioned the exceptions that allow returns at the beginning of a function that check preconditions.

True, and that is indeed a good example that you're citing. I still think my version is easier to understand, but hey - I did the work on it, so it might as well just be confirmation bias ;-)

I think that the maybe-best implementation is:

function foo() { $return = null;

 if (expr)
 {
     [...]
     $return = $something;
 }

 return $return;

}

2013-09-05 11:09 időpontban David Deutsch ezt írta:

Not sure about that. In some cases I have seen so far, there were quite a number of elseifs. My goal was to get out of the function as soon as possible - continuing to iterate through all the cases you already know won't apply doesn't seem like a good use of resources to me.

In my example if the first expression returns with false no other iteration made just return with the predefinied null variable.

I also don't think that part of it was controversial - it was more about that up until now, returns were in general either at the start or end of a function and what I introduced used some more in the middle of a function. So in your example, you would still kinda-sorta hunt for where the $return is set in the first place.

I really hate negated expressions and mid-block returns but this is not my code I just wrote some plugins to fit RC in my environment.

On Thu, Sep 5, 2013 at 12:11 PM, David Deutsch skoremail@gmail.com wrote:

...

I really hate negated expressions and mid-block returns but this is not my code I just wrote some plugins to fit RC in my environment.

The way I'm applying them right now, I don't particularly enjoy them either, but I do think they're the lesser of two evils. As discussed before - what you want to have, in the end, is a separation into functions. I think disentangling those large if statements is a good first step towards that.

OK, after long discussions here and in the pull requests on github, can we try to conclude where that refactoring is supposed to go?

For what we have seen and discussed, here's what I would like to happen and not to happen:

• no intermediate returns but clear if/elseif/else clauses. If the

individual clauses exceed a certain number of lines (let's say, 20 lines) they should become individual functions/methods.

• allow immediate returns at the beginning of a function when

pre-conditions are not met.

• only re-arrange blocks of variable assignments or function call

blocks with additional lines where they're really hard to understand.

e.g. these should not be torn apart with blank lines:

$rcmail = rcmail::get_instance();
$identity = $rcmail->user->get_identity();
$identities_level = intval($rcmail->config->get('identities_level', 0));

• no one-line if blocks without braces.

• all if/for/while blocks should get braces.

• else if / else clauses start on a new line. Not "} else {"

• object/array function arguments should start on the same line as the

function call. That saves an indentation level.

Good: dialog.dialog({ ... });

$this->api->output->button(array( ... ));

Bad: dialog.dialog( { ... } );

$this->api->output->button( array( ... ) );

• Javascript: opening brace for anonymous functions are on the same

line. No space between 'function' and brackets.

May I ask you to re-do or update the pending pull requests according to this suggestion? And in order to save your precious time, you should wait for approval of the first pull request before moving on.

Thanks a lot for you efforts and all the inputs!

Regards, Thomas

On Saturday, September 7, 2013, David Deutsch wrote:

Hi Thomas,

Yes, that was what I was trying to get at with my last post - wrapping up the decisions so we can move along ;-)

I agree with all of the points raised except one - having else/elseifs always start on a new line. First, let me tell you where I agree with you - there really are some "long" if/else blocks (exceeding a dozen or so lines)

• for those, I would agree that in the current state of the codebase,

pulling apart the structure a bit with new lines can help readability. In the end, I think the best way to go would be a terse (no linebreaks) syntax and (again in agreement with what you wrote) extra methods, but Alec has vetoed new methods during cleanup, so I cannot go there at the moment. Of course, I'd be more than happy to have that option, after all ;-)

But, again - I can't help myself, so still trying to argue here. In my opinion, a linebreak like that, applied consistently, would absolutely give other developers working with the codebase pause and even if we assumed for the sake of argument that the general readability in both possible scenarios is more or less equal, in a worst case scenario, the linebreaks route is simply more confusing. My go-to example would be this:

https://github.com/roundcube/roundcubemail/blob/master/plugins/managesieve/l... vs.

https://github.com/daviddeutsch/roundcubemail/blob/905ef62978970e27818656610...

I'm sorry but I can't follow you here. What we see here is the processing of different cases in individual if/elseif blocks on the same indentation level. What's so wrong about this? IMO your optimizations are way less readable because they add an additional indentation level (which you aim to avoid with intermediate returns, don't you?) So except the reformatting of the arguments array of raise_error() calls, I definitely don't see anything that needs to be improved in order to make the code more understandable to others, sorry.

Now, you might rightfully say that this is an extreme example that needs to be solved differently anyhow. But that gets back to my earlier point - the majority of if/else statements already are terse enough to be understood at one glance, examples:

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

I don't see how the extra linebreaks would improve the situation here. So following that reasoning, we end up with fewer and fewer cases where the newline really makes sense... and thus I would think it also makes less and finally no sense as a general rule. It's either ill advised or so obscure that it could just as well be personal taste.

You might be right with the simple examples above and I partially agree that this waists some vertical space with single-line if clauses. But we want to make our code consistent, right? And I think the extra line breaks generally contribute to the readability because they visually separate the blocks which is desirable in the average case.

On the one hand you suggest that variable assignments and such should be torn apart with empty lines in between for better readability and on the other hand you don't accept our way to visually separate if/else blocks by moving the elseif/else statement to a new line?

(I would also like to add the curiosity that some of the above examples are precisely the same in the original code - just with added brackets. ;-) )

To sum up:

• Extremely uncommon syntax (while I'm trying to change the codebase into

being more approachable to outsiders)

Yes, it might not fit the PSR-2 guidelines but come on, if somebody is unable to read our code because of that, he/she would not qualify as a useful contributor to Roundcube anyways. I know, I'm probably risking to become an arrogant dick on your eyes but I'm just naming the facts.

• In the few situations where it is helpful, it mostly covers other flaws

(code smells like overly long functions)

• In most situations, it's not helpful at all

It's definitely not meant to cover flaws and we already agreed that too big blocks should be refactored into functions or in another meaningful way. But that's not an argument for or against the said new lines.

And once again - I'm not trying to force your hand here or anything and I think I have shown that I'm usually absolutely happy to accept an opinion contrary to my own and incorporate it. In this case however, I'm pretty sure it's not just my taste against yours. My - I think informed - opinion is that you're making a bad call on this. I further think that one or two passes down the cleanup, these won't matter nearly as much as they seem to matter now.

Finally - it's a cleanup, some things are supposed to hurt. That's just the way it is. Part ye with ye olde ways and such ;-)

I'm willing to bear some pain when I see a benefit. But I more and more have the feeling that we got ourselves into a bikeshed discussion. And I'd therefore appreciate other opinions on this. If I'm really the only dickhead who hates the PSR-2 if/else syntax, I'll step back.

---

For the "no returns in the middle of a function" requirement, I would like to have one clarification. As an example, this function here ends with a rather long if and a small else:

https://github.com/daviddeutsch/roundcubemail/blob/20de28b9e94a5f001161ca49f...

I would usually invert the if and in that "return $fields['user'];". Would that be permissible?

I tend to say no on this. But again, I'd like to hear others speaking up for or against single exit points. For me these are like GOTO commands. They suddenly make the program jump out of the current block which - especially if it's a function - has a clear beginning and an end.

In general, I'm having a hard time finding the right ranges here. Maybe we could set a rough limit on length of function or maybe you could give me one or two examples from the existing cleanups that illustrate the point better?

I previously tried to give a limit of 20 lines for if/else blocks. But that was just a wild estimation without looking at specific examples. But since Alec vetoed on function refactoring, there's no point in stating a number right now.

Thanks a lot for your patience in discussing this though!

Regards, Thomas

On Tue, Sep 10, 2013 at 7:35 PM, A.L.E.C alec@alec.pl wrote:

On 09/06/2013 01:52 PM, Thomas Bruederli wrote:

...

• Javascript: opening brace for anonymous functions are on the same

line. No space between 'function' and brackets.

I'd add that I prefer

function() {

over

function(){

I actually meant function() vs. function ()

And I'm definitely on your side for what you just proposed.

~Thomas

On 2013-09-11 11:06, Reindl Harald wrote:

what i personally *never* understood is why the whole world is writing one of both unreadable code-styles at all

...

This is a 40-year-old debate over style. Having it again here cannot benefit the Roundcube project in any way.

• R.

[__ Robert Sheldon [__ No Problem [__ Information technology support and services [__ (530) 575-0278

Having it again here cannot benefit the Roundcube project in any way.

Indeed, I hope I did not make the impression that I wanted to have the basic style discussion that has turned many mailing lists first into hot pits of hell and then tumbleweeds. I was just trying to find a consensus to build on and got a bit carried away trying to influence the decisionmaking ;-)

In the end, there are a lot of pros and cons for various ways of doing things. The important thing is making decisions while remaining open to change. We have come to a decision and that's what counts right now.

On Wed, Sep 11, 2013 at 8:29 PM, Reindl Harald h.reindl@thelounge.netwrote:

Am 11.09.2013 20:16, schrieb Rob Sheldon:

...

On 2013-09-11 11:06, Reindl Harald wrote:

...

what i personally *never* understood is why the whole world is writing one of both unreadable code-styles at all

...

This is a 40-year-old debate over style. Having it again here cannot

benefit the Roundcube project in any way

maybe you answered to a different thread / topic as i did............

---

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

Alright, got the first round in rather quickly, Alec commented some more two days ago and I just pushed through a couple more commits.

I think there are only two situations left where we are still discussing the details, but I think we're getting close to finishing this. If anybody still has broader points to address, I think this is now the time, otherwise, I think we have ourselves a first good-to-go PR.

I must say that I'm quite happy with how things are turning out, I even think this is starting to change my own preferences on code style a little

• dialing back on one-liners, mainly ;-)

Thanks to everybody who has helped and weighed in so far - and thanks in particular for the patience!

-David

On Thu, Sep 12, 2013 at 9:15 AM, David Deutsch skoremail@gmail.com wrote:

Alright, I think the first pull request is pretty much up to our specification: https://github.com/roundcube/roundcubemail/pull/109

If you find anything that I've missed, just let me know!

On Wed, Sep 11, 2013 at 8:31 PM, David Deutsch skoremail@gmail.comwrote:

...

...

Having it again here cannot benefit the Roundcube project in any way.

Indeed, I hope I did not make the impression that I wanted to have the basic style discussion that has turned many mailing lists first into hot pits of hell and then tumbleweeds. I was just trying to find a consensus to build on and got a bit carried away trying to influence the decisionmaking ;-)

In the end, there are a lot of pros and cons for various ways of doing things. The important thing is making decisions while remaining open to change. We have come to a decision and that's what counts right now.

On Wed, Sep 11, 2013 at 8:29 PM, Reindl Harald h.reindl@thelounge.netwrote:

...

Am 11.09.2013 20:16, schrieb Rob Sheldon:

...

On 2013-09-11 11:06, Reindl Harald wrote:

...

what i personally *never* understood is why the whole world is writing one of both unreadable code-styles at all

...

This is a 40-year-old debate over style. Having it again here cannot

benefit the Roundcube project in any way

maybe you answered to a different thread / topic as i did............

---

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

I'm telling you that it's my condition to helping out that my contributions are properly respected in the commit history of the project. I'm pretty sure every single other developer contributing would see it the same way. I'm offering my work and if it's more important to you to not be a little annoyed in a rare use case than doing this the proper way then, well, then I know what to think about that.

Messy commits like the one you mentioned will of course be less frequent going forward and besides I think that's a particularly weak point. You see my username, you know who I am, you can see what the changes were. You're a programmer, you're dealing with data. If you're so annoyed by it, you can simply filter me out by username if there is anything in particular you want to check on.

Thank you for your input. I understand the limited concerns, but while I have so far been happy to accept my opinion being overruled, this is one of the few things that I will not compromise on.

I further think "dont tell me meta data is more important than data" sums up my technical argument quite neatly. Just make your commit log printer ignore any commits from my username for a set time period and the problem is solved ;-)

And: If you check on the other PRs, you will find them containing far less commits with more changes compounded into one commit. I'd be happy to further drive that number down in the future - the only reason why I kept it like this for this one PR was so that it could be followed and we could discuss it in detail. But that is unrelated to my requirement of getting proper credit ;-)

On Wed, Sep 18, 2013 at 12:52 PM, Cor Bosman cor@xs4all.nl wrote:

I think you both have a valid point, but I tend to lean towards Alec's point.

These PRs do not add any functionality to the code. They're 'merely' code cleanups. I put that between quotes because it's awesome work and i for one am very happy someone is taking that on. But I agree with Alec that committing every single section of cleanup as a new commit is kind of excessive. So I hope David will agree to send cleanup PRs as single (or smaller) commits.

On the other hand, if both of you remain resolved in your opinion, then for heaven's sake, dont tell me meta data is more important than data. Just accept the googol of commits and be happy with the code itself.

Cor

---

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

David Deutsch wrote:

So you're saying a clean commit history is more important than giving me proper credit where it is due? Again, that is not acceptable. Besides - why is the commit history that important to begin with?

I have to agree with Alec here. Commit history is important for us and so are proper commit messages. I want to be able to find out who wrote a particular line of code and why it is like it is. That includes tracking changes (preferably with ticket numbers in the commit message) to find the reason why I better do not change that or what circumstances are to consider when changing something. That's the reason why I initially wasn't really keen on code refactoring because it doesn't change the functionality but bloats the history.

Of course the arguments for cleaning up the code are strong enough to sacrifice that history let's still try to minimize it. Preferably we want one single commit per PR that says "Code cleanup by David Deutsch" or something. I know that there'll be more but what we currently have in the pending PRs of yours is rather messy and not helpful. I admit that's primarily because of all the discussions we had to reach the agreement which we now have and I also understand that other PRs will be way shorter.

It's definitely not about not wanting to give you the credits you deserve for all your hard work, please don't get us wrong on this. If you have other propositions how we can credit your work, please let us know.

But still I kindly request you to re-create the pull requests with one commit per processing step (as proposed in [1]) and with descriptive commit messages.

Kind regards, Thomas

[1] https://gist.github.com/daviddeutsch/6376013

On Wednesday 18 September 2013 14:04:45 David Deutsch wrote:

So it goes.

Sorry guys, I usually stay silent on this list, but I can not stay silent now.

We have this huge thread with long discussions and many compromises. Everybody did their best to make this work and now after so much work has already been put into it, we just want to throw everything away and stop all future work on the cleanup over this last little obstacle of commit history?

I think it is worth investing a little more time now to solve also this last issue so that not everything was in vain.

David is a motivated and respectful guy who has been very cooperative so far stated his intentions to contribute further beyond code cleanups. We should not let him go like this.

He obviously cares a lot about receiving the well-deserved recognition for his work and sees this mainly in the maintained traces of his work in the form of single commits.

Alec and Thomas on the other hand have to work with the code daily and have to solve many tricky problems which often involve going through the commit history. It is also understandable that they don't want to complicate this common task.

Both parties obviously have a legitimate interest here. Let us try to find a way to bring those together.

David, do you think there would maybe be some other way for you to receive the recognition that you rightfully deserve? Maybe it could be in some other form than a full commit history on your first PRs?

I think both parties to the dispute agree that the later PRs are less problematic because they will be a lot cleaner and will contain much less commits. So we only need to find a solution for the first ones.

Kind Regards, Torsten

David, be reasonable. I think Alec and Thomas have very sensible objections to your commits.

As an example:

https://github.com/roundcube/roundcubemail/commit/454f7a93790375e5076324b473...

That is a single commit removing 1 line, with a comment of 'yup, makes sense'. What makes sense? I feel im missing some part of a discussion just reading that commit history.

You feel you're not being properly recognised if you submit a PR with sensible commits and sensible commit comments? This isnt about commit counting but about substance. The substance is great, but it is being diluted in an avalanche of commits that are all trivial and unclear.

I would much rather see a PR with 1 commit that contain a group of code cleanups, with a commit comment like: "Code cleanup by David Deutsch".

Anyways, i really hope you'll re-commit your PRs because this unfortunate argument seems solvable.

Cor

On Wednesday 18. September 2013 15.23.56 David Deutsch wrote:

Credibility? Eternalize? What? Look - I'm just a FOSS coder and I don't care how "professional" or whatever I come across. What I do care about is an /honest/ track record that can be seen in my github profile, amongst other things. I would like to help out in other projects as well, eventually, and I want to be able to offer an honest, cohesive picture of how past efforts went about. That's why I showed you what I did for RedBean

• to give you a direct view into how it went down in another example. If I

propose help to other projects, I don't think they would care much about how "professional" I am, but they would very much appreciate an honest picture of the process.

I'm mostly lurking on this list at the moment, having made an enquiry a few months ago about something that I've not been able to prioritise (more below), but this thread is too painful to read without commenting.

In principle I also am against the excessive rebase culture that a lot of Free Software projects employ. The joke about this culture is that in its most extreme form one wouldn't bother having more than a single commit in a repository, and that commit would be accompanied by a message reading "Perfection!", "All done!", "Project complete!" or "Nailed it!"

That you also see projects *making* version control software insisting on rebasing or collapsing changesets, even though rebasing may be frowned upon and collapsing changesets may involve advanced functionality, could be considered akin to hypocrisy: people making tools to manage the information in software development insisting that such information be thrown away. (Please note that I'm not saying anything about Roundcube's commit or contributions policy here.)

However, one should respect that projects do have commit policies for good reasons. Some of these policies are infuriatingly strict: the Mercurial project, for example, generally wants a single commit for enhancements, bug fixes and new features (even though no-one in their right mind would do the work in a single commit "for real"), and the commit message must adhere to a specific format; all of this is on top of other policies one may or may not like (line lengths, discouragement of comments, obligatory tests, discouragement of new tests, obligatory documentation, and so on). It can take several iterations to get something that the core developers will accept.

On the one hand, it can seem like people are just making life hard for casual contributors. I am aware of one project controlled by a large corporation who apparently makes contributing very much like a "ring of fire" experience perhaps even more extreme than what I have described above. When people who are paid to work on a project make more work for volunteers, one can legitimately question their motives.

On the other hand, it is understandable that core developers do not wish to readily take on more work that other people have thrown over the wall, giving those core developers code to maintain forever while the contributors enjoy the benefits of their work in the resulting product, with the contributed code magically bug-fixed and updated for any and all of the architectural changes and transformations that might come about.

As others have pointed out, your work will always be available in the form in which you made it available if you continue to publish your repositories/branches. Those who you wish to convince about the substance of your work will still be able to see it and appreciate your efforts. But you should also appreciate that those who have to maintain your contributions should also get to choose how they can work with those contributions. Denying those people any choice sends a signal that may be interpreted negatively by others, regardless of whether words like "professional" are in their vocabulary.

I think it is great to see your enthusiasm to improve Roundcube, and being much more of a Python developer than someone who uses PHP, your work appears to be beneficial to people like me. Please don't squander this opportunity to see good work done by attaching a price to your contributions that may end up with only you bearing the cost.

Paul

P.S. My original business on this list was to inquire about accessibility support in Roundcube. If anyone has any thoughts on the topic (whether Roundcube is perceived to be sufficient/deficient, whether work could be done), I'd be happy to revive that thread.

This is a very simple matter.

https://github.com/roundcube/roundcubemail/pull/109

This is where the work was done and this is my primary record of what happened. There are a lot of comments in there, discussions about how to do the cleanup and a lot of commits:

https://github.com/roundcube/roundcubemail/pull/109/commits

The core of the PR are only about 12 commits, plus 45 further commits to adjust to additional requests by the lead developers (on this list and on the PR discussion). I have spent my time negotiating those changes and to me, this record of the process is an important part of my work.

THIS is what I want to have on record (and yes, on the main repo) if we are to move forward. I want this PR to become merged, not any other in its stead. While it might be an uncomfortable or slightly chaotic record, it is the honest record of what happened and if my work is to be accepted, I want it accepted in the way it came about. If it is more important that it should be neat and tidy than honest then I disagree and take my work elsewhere. I don't want my work whitewashed and I'm getting a weird feeling that maybe that record in itself is a problem.

We can discuss finer points - Sure, some commit messages could be more precise (and I have offered to change the messages), but as you see from the discussion, mostly they were meant as replies to requests. I have tried making most of the commits descriptive yet with multiple tiny requests spread over days, you sometimes run out of steam on that. Anyways, that doesn't matter as it can be changed. So far, this hasn't even been acknowledged. Heck, you could probably even persuade me to git rebase a couple of those tiny commits away.

What I don't need is to be told that this is some actual technical challenge to have a couple dozen more commits on record, because that's simply bullshit. As I've said before: you're developers, stop pretending your commit history is some precious flower that cannot possibly be tainted. It's pure fantasy that there is any problem here and I wish everybody would stop making up more and more reasons that are just as ludicrous. You know what actually happens if somebody stumbles over this? It takes them a couple of seconds until they see "oh, it's a bunch of cleanup", then they move on. End of story.

Nor do I need to be told how this is a bad example to other developers because the work was chaotic or because you see yourself on some slippery slope where suddenly people no longer write proper commit messages. Bullshit again, because this is a question of whether you can keep the boat steady with a little turbulence. Not my problem, YOUR problem, but you want to make it mine.

I also don't need to be told how this comes across as unprofessional or how it would be so terrible to defend having a tiny bit of unprofessionalism on your record.

This is a very simple matter. I have offered my work and have now made a request that you accept some imperfections in the process for my sake. The response was that you like my work, but would rather not return a small favour. Instead of bearing a little discomfort yourself and maybe having to stand for a little bit of chaos publicly, you would rather have me make further concessions. I have offered concessions that I /could/ make, but nope, you would rather have me make the concessions that you want.

How you imagine that that is a premise for a working relationship going forward is beyond me.

I don't need to be here and I don't need to work on this. I have put a lot of work into this PR and six more PRs that were lined up, partially involving more work than this one.

But I would rather walk away from it than have my work distorted.

It would be a shame and a waste, but it's simply not my decision.

On Wed, Sep 18, 2013 at 7:50 PM, stephane martin stef.martin@gmail.comwrote:

Painful ? It's actually quite refreshing after a hard work day. Enjoyable.

Regards, Stephane

PS : I'd be very interesting to talk about general accessibility/ergonomics too.

On Wed, Sep 18, 2013 at 7:14 PM, Paul Boddie paul@boddie.org.uk wrote:

...

On Wednesday 18. September 2013 15.23.56 David Deutsch wrote:

...

Credibility? Eternalize? What? Look - I'm just a FOSS coder and I don't care how "professional" or whatever I come across. What I do care about

is

...

an /honest/ track record that can be seen in my github profile, amongst other things. I would like to help out in other projects as well, eventually, and I want to be able to offer an honest, cohesive picture

of

...

how past efforts went about. That's why I showed you what I did for

RedBean

...

• to give you a direct view into how it went down in another example.

If I

...

propose help to other projects, I don't think they would care much about how "professional" I am, but they would very much appreciate an honest picture of the process.

I'm mostly lurking on this list at the moment, having made an enquiry a few months ago about something that I've not been able to prioritise (more below), but this thread is too painful to read without commenting.

In principle I also am against the excessive rebase culture that a lot of Free Software projects employ. The joke about this culture is that in its most extreme form one wouldn't bother having more than a single commit in a repository, and that commit would be accompanied by a message reading "Perfection!", "All done!", "Project complete!" or "Nailed it!"

That you also see projects *making* version control software insisting on rebasing or collapsing changesets, even though rebasing may be frowned upon and collapsing changesets may involve advanced functionality, could be considered akin to hypocrisy: people making tools to manage the information in software development insisting that such information be thrown away. (Please note that I'm not saying anything about Roundcube's commit or contributions policy here.)

However, one should respect that projects do have commit policies for good reasons. Some of these policies are infuriatingly strict: the Mercurial project, for example, generally wants a single commit for enhancements, bug fixes and new features (even though no-one in their right mind would do the work in a single commit "for real"), and the commit message must adhere to a specific format; all of this is on top of other policies one may or may not like (line lengths, discouragement of comments, obligatory tests, discouragement of new tests, obligatory documentation, and so on). It can take several iterations to get something that the core developers will accept.

On the one hand, it can seem like people are just making life hard for casual contributors. I am aware of one project controlled by a large corporation who apparently makes contributing very much like a "ring of fire" experience perhaps even more extreme than what I have described above. When people who are paid to work on a project make more work for volunteers, one can legitimately question their motives.

On the other hand, it is understandable that core developers do not wish to readily take on more work that other people have thrown over the wall, giving those core developers code to maintain forever while the contributors enjoy the benefits of their work in the resulting product, with the contributed code magically bug-fixed and updated for any and all of the architectural changes and transformations that might come about.

As others have pointed out, your work will always be available in the form in which you made it available if you continue to publish your repositories/branches. Those who you wish to convince about the substance of your work will still be able to see it and appreciate your efforts. But you should also appreciate that those who have to maintain your contributions should also get to choose how they can work with those contributions. Denying those people any choice sends a signal that may be interpreted negatively by others, regardless of whether words like "professional" are in their vocabulary.

I think it is great to see your enthusiasm to improve Roundcube, and being much more of a Python developer than someone who uses PHP, your work appears to be beneficial to people like me. Please don't squander this opportunity to see good work done by attaching a price to your contributions that may end up with only you bearing the cost.

Paul

P.S. My original business on this list was to inquire about accessibility support in Roundcube. If anyone has any thoughts on the topic (whether Roundcube is perceived to be sufficient/deficient, whether work could be done), I'd be happy to revive that thread. _______________________________________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/mailman/listinfo/dev

---

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

Hi.

I totally agree with Cor: The work done so far seems to be a very good contribute to this project, so first at all: Thanks alot to everybody involved in this discussion and process, and especially to David who did this work.

But when it comes to discussion about code style (and that was this topic all about) one has to face the fact that commit messages are part of the coding style and should get the same respect and care like the code itself or the documentation of the code and the project.

Roundcube is a really great project, and several pieces have come together to get it where it is today:

• A really good code base
• Quick bugfixes and releases
• A really good plugin concept, making it easy to enhance the feature set
• And last but totally not least: Project maintainers that do a really

great job. By their work they assure all the great features, new developments, and a codebase that empowers people to contribute to this project.

In order to keep this high quality they have to set up certain rules of contribution everybody has to accept.

So please: Rework your commits and pull-request them again. The hardest work of cleaning up is already done, re-doing the commits will not take much time but make your work come alive.

Kind regards, Daniel Rauer

On 09/18/2013 02:58 PM, Cor Bosman wrote:

David, be reasonable. I think Alec and Thomas have very sensible objections to your commits.

As an example:

https://github.com/roundcube/roundcubemail/commit/454f7a93790375e5076324b473...

That is a single commit removing 1 line, with a comment of 'yup, makes sense'. What makes sense? I feel im missing some part of a discussion just reading that commit history.

You feel you're not being properly recognised if you submit a PR with sensible commits and sensible commit comments? This isnt about commit counting but about substance. The substance is great, but it is being diluted in an avalanche of commits that are all trivial and unclear.

I would much rather see a PR with 1 commit that contain a group of code cleanups, with a commit comment like: "Code cleanup by David Deutsch".

Anyways, i really hope you'll re-commit your PRs because this unfortunate argument seems solvable.

Cor

---

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

Well, those two are not mutually exclusive. Things like the body formatting are at the core of the system and as such somewhat susceptible to fall prey to inertia more quickly than things on the outskirts. Then again, I could not find the discussion you refer to, so I can only speculate here.

I would love to see my efforts blossom into directly supporting or at least aiding the process of keeping RCM up with the demands of the userbase by reducing that inertia. If I have done my homework correctly, it should at the very least make it easier to contribute to the project and keep contributions cohesive without increasing technological debt. In the best case scenario, we will end up cutting down on existing technological debt.

Finally - I don't think that the message count is an indication of focus. Most of the messages in this thread are from me, anyhow. So if anything, they're proof that I like to post a lot of status updates.

-David

On Wed, Sep 11, 2013 at 11:09 PM, Michael Heydekamp listuser@freexp.dewrote:

Am 11.09.2013 20:06, schrieb Reindl Harald:

...

what i personally *never* understood is why the whole world is writing one of both unreadable code-styles at all

And I'd wish that some day a discussion about formatting the *body* of a Roundcube message (which does affect all Roundcube users plus their recipients) would get the same attention as a discussion about formatting the Roundcube *code* (which does affect almost no Roundcube users nor their recipients at all).

But the former discussion had been actively dropped after just a few messages. The discussion about formatting the code deserved 66 messages up to now, and there may still be more to come.

Wow and <sigh>.

Cheers,

Michael Heydekamp Co-Admin freexp.de Düsseldorf/Germany _______________________________________________ Roundcube Development discussion mailing list dev@lists.roundcube.net http://lists.roundcube.net/mailman/listinfo/dev

Am 11.09.2013 23:58, schrieb Reindl Harald:

Am 11.09.2013 23:09, schrieb Michael Heydekamp:

...

And I'd wish that some day a discussion about formatting the *body* of a Roundcube message (which does affect all Roundcube users plus their recipients) would get the same attention as a discussion about formatting the Roundcube *code* (which does affect almost no Roundcube users nor their recipients at all).

But the former discussion had been actively dropped after just a few messages. The discussion about formatting the code deserved 66 messages up to now, and there may still be more to come.

how many raw-messages do you want to prove you that roundcube currently has *far* the best message renderer of all mail-clients?

None, as I'm not talking about rendering/displaying incoming messages. And as Roundcube is almost perfect in this regard, this is one of a major number of reasons why I'm using it.

I'm talking about formatting messages upon composing/quoting instead.

so what is your exactly problem

The last time I tried to point to a problem with regards to body formatting upon composing/replying in this list, I got just one response: "Sorry, I'll not read this, I'm also a little bit tired about this." (May 10th, 2013)

So I decided not to bother the devs anymore with these issues. Code formatting issues seem to be more important (at least they deserved 18 responses from the devs, not counting responses from non-devs), even one white space more or less is of importance apparently (although the relevance to the user is not even close to zero, but exactly zero).

So be it. I have to accept that, and just sit and wait.

and you realize how complex e-mail is at the end of the day

As I have been the maintainer of a MUA (DOS/16) myself, you can trust me that I'm more than aware of that.

Cheers,

Michael Heydekamp Co-Admin freexp.de Düsseldorf/Germany

Am 12.09.2013 20:43, schrieb A.L.E.C:

On 09/12/2013 08:38 PM, Michael Heydekamp wrote:

...

So I decided not to bother the devs anymore with these issues. Code formatting issues seem to be more important (at least they deserved 18 responses from the devs, not counting responses from non-devs), even one white space more or less is of importance apparently (although the relevance to the user is not even close to zero, but exactly zero).

Not more important, but just simpler. It was much bigger effort to understand your requests/issues and investigate them.

That's for sure, right. And as I said:

And I'd wish that some day a discussion about formatting the *body* of a Roundcube message (which does affect all Roundcube users plus their recipients) would get the same attention as a discussion about formatting the Roundcube *code* (which does affect almost no Roundcube users nor their recipients at all).

Should this day come at some time, I'd be glad if you would let me know. Till then, I keep my feet still with regards to those issues (unless any regressions might occur which need to be fixed instantly).

Cheers,

Michael Heydekamp Co-Admin freexp.de Düsseldorf/Germany

David Deutsch wrote:

Good question! (Also: shame on me for not finding those guidelines ealier O_o .../awkward/)

I think the basic recommendation that PSR-1 makes on this are pretty sound: Make a choice and stick with it. As a personal preference, I use StudlyCaps for classes (this is a 'must' in PSR-1), camelCase for methods and under_scores for variables. I find that that helps separate the three concepts, even though it might seem like "hey, why do you use three ways of doing something". Because it's three separate concepts!

We made our choice and we're going to stick with it. Thus use underlines and no caps. And we're not doing PSR-1 here.

From what I've seen in the plugins so far, it seems like RCM pretty consistently uses lowercase_underscored for all three.

Sidenote: My IDE actually gives me a lot of funny warnings about the class names in RCM - something along the lines of "there is probably a conceptual error here, somewhere" I always imagine it standing before me with eyes twitching "dude, this aint right!" ;-)

I'm wondering what your IDE means by "conceptual errors".

Sidenote 2: The Guidelines references PEAR standards. I used to really dislike those, particularly for the ~80 character line limit, which seemed silly ("Oooh, your b/w terminal can only do 80 characters per line!?"). Fast forward a couple of years and I absolutely love an 80 char line limit. It's like an addiction to shortness.* There are a couple of outdated things in there and I might forever think that 4 spaces is silly, but yeah, not as bad today ;-)

Agreed. After this cleanup we should definitely rewrite the guidelines.

*Sidenote 3: This also helped me come around on Namespaces, really. "use... as" REALLY helps keep lines short when using classes and particularly when calling static methods.

We're not there yet.

~Thomas