On Mon, Sep 2, 2013 at 12:35 PM, David Deutsch <notifications@github.com>wrote:

> In plugins/acl/acl.php:
>
> > @@ -86,14 +91,17 @@ function acl_autocomplete()
> >          $this->load_config();
> >
> >          $search = rcube_utils::get_input_value('_search', rcube_utils::INPUT_GPC, true);
> > -        $sid    = rcube_utils::get_input_value('_id', rcube_utils::INPUT_GPC);
> > -        $users  = array();
> > +
>
> Not exactly sure how to respond to that...
>
> Look - This your project and I can only make recommendations. I understand
> the proposal here isn't like your style - that's kind of the whole point [image:
> :wink:]
>
> I think it might make more sense to discuss the merit of the suggestion
> itself, not its relation to the existing codebase. Because if we continue
> to compare it with the way you did it for years, we end up with no changes
> at all.
>
> This example to me is pretty straight forward, as I've said above. The
> reason why the statements were clumped together was because in a lot of
> places in the code, you just create tons and tons of variables that are
> reused. If that's the style then yes, leaving it like that is *consistent*.
> It is however actually a problem in the codebase. Of course, in this
> example it is only three lines, but the underlying remedy is the same: Get
> the things you need where you actually need them. I have already moved lots
> of variable declarations into if statements when they weren't used except
> for that one case. In this example, the $users declaration belongs to the
> foreach, but needs to be placed outside of the if statement since we use it
> as an empty array if ldap isn't intialized. It is also a separate operation
> from calling in environmental variables. For me, all that is the reason for
> putting it right outside of the if and not together with the statements
> above.
>
> I would be happy to discuss any reasoning to the contrary, but "we used to
> do it differently"... doesn't really give me anything to discuss.
>
There's certainly some nostalgic and emotional component in this but even
more important for us is to keep the impact of the changes in this
refactoring as little as possible. Just for the sake of keeping existing
patches applicable. We have some patches in trac where we encouraged people
to maintain patches for features we didn't want to become part of the core.
And there's an unknown number of patches out there that are applied after
each update. I simply don't want to stop them working just because of some
controversial whitespace added.

So grouping of variable assignments or variable + if/for blocks quickly
becomes a personal taste and real objective arguments for the why and the
how are hard to find. Thus I suggest that existing grouping should not be
changed unless the code is totally unreadable. And I don't think that our
codebase is really that bad.

Regards,
Thomas