Mass migration to new syntax - PRO or CON?

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Purodha Blissenbach
A quick asking around among programmers here gives 7:1 pro array() and
con [] syntax.
The latter is seen as less readable. Is there a technical advantage of
[] over array()
that we should harvest?

Purodha

On 12.02.2016 21:06, Ricordisamoa wrote:

> Il 12/02/2016 20:26, Legoktm ha scritto:
>> Hi,
>>
>> On 02/12/2016 07:27 AM, Daniel Kinzler wrote:
>>> Now that we target PHP 5.5, some people are itching to make use of
>>> some new
>>> language features, like the new array syntax, e.g.
>>> <https://gerrit.wikimedia.org/r/#/c/269745/>.
>>>
>>> Mass changes like this, or similar changes relating to coding
>>> style, tend to
>>> lead to controversy. I want to make sure we have a discussion about
>>> this here,
>>> to avoid having the argument over and over on any such patch.
>>>
>>> Please give a quick PRO or CON response as a basis for discussion.
>>>
>>> In essence, the discussion boils down to two conflicting positions:
>>>
>>> PRO: do mass migration to the new syntax, style, or whatever, as
>>> soon as
>>> possible. This way, the codebase is in a consistent form, and that
>>> form is the
>>> one we agreed is the best for readability. Doing changes like this
>>> is
>>> gratifying, because it's low hanging fruit: it's easy to do, and
>>> has large
>>> visible impact (well ok, visible in the source).
>> I'll offer an alternative, which is to convert all of them at once
>> using
>> PHPCS and then enforce that all new patches use [] arrays. You then
>> only
>> have one commit which changes everything, not hundreds you have to
>> go
>> through while git blaming or looking in git log.
>>
>>> CON: don't do mass migration to new syntax, only start using new
>>> styles and
>>> features when touching the respective bit of code anyway. The
>>> argument is here
>>> that touching many lines of code, even if it's just for whitespace
>>> changes,
>>> causes merge conflicts when doing backports and when rebasing
>>> patches. E.g. if
>>> we touch half the files in the codebase to change to the new array
>>> syntax, who
>>> is going to manually rebase the couple of hundred patches we have
>>> open?
>> There's no need to do it manually. Just tell people to run the phpcs
>> autofixer before they rebase, and the result should be identical to
>> what's already there. And we can have PHPCS run in the other
>> direction
>> for backports ([] -> array()).
>>
>> But if we don't do that, people are going to start converting things
>> manually whenever they work on the code, and you'll still end up
>> with
>> hundreds of open patches needing rebase, except it can't be done
>> automatically anymore.
>>
>>> My personal vote is CON. No rebase hell please! Changing to the
>>> syntax doesn't
>>> buy us anything.
>> Consistency buys us a lot. New developers won't be confused on
>> whether
>> to use [] or array(). It makes entry easier for people coming from
>> other
>> languages where [] is used for lists.
>
> Objection: other languages may use [] for lists, but array() is more
> similar to {} used for hash tables.
>
>>
>> I think you're going to end up in rebase hell regardless, so we
>> should
>> rip off the bandaid quickly and get it over with, and use the
>> automated
>> tools we have to our advantage.
>>
>> So, if we're voting, I'm PRO.
>>
>> -- Legoktm
>>
>> _______________________________________________
>> Wikitech-l mailing list
>> [hidden email]
>> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l


_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Jon Robson-2
In reply to this post by Chad
On 12 Feb 2016 12:44 p.m., "Chad" <[hidden email]> wrote:
>
> On Fri, Feb 12, 2016 at 9:14 AM Chad <[hidden email]> wrote:
>
> > On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler <[hidden email]>
> > wrote:
> >
> >> CON: don't do mass migration to new syntax, only start using new styles
> >> and
> >> features when touching the respective bit of code anyway. The argument
is

> >> here
> >> that touching many lines of code, even if it's just for whitespace
> >> changes,
> >> causes merge conflicts when doing backports and when rebasing patches.
> >> E.g. if
> >> we touch half the files in the codebase to change to the new array
> >> syntax, who
> >> is going to manually rebase the couple of hundred patches we have open?
> >>
> >>
> >> As can be seen on the proposed patch I linked, several of the long term
> >> developers oppose mass changes like this. A quick round of feedback in
the
> >> architecture committee draws a similar picture. However, perhaps there
are

> >> compelling arguments for doing the mass migration that we haven't heard
> >> yet. So
> >> please give a quick PRO or CON, optionally with some rationale.
> >>
> >> My personal vote is CON. No rebase hell please! Changing to the syntax
> >> doesn't
> >> buy us anything.
> >>
> >>
> > CON, for all the reasons you mentioned. Also: style only changes are
pain

> > when you're
> > trying to annotate/blame a particular line of code.
> >
> > ESPECIALLY for something so silly as array formatting which gains us
> > *absolutely nothing*
> >
> > -Chad
> >
>
> I change my vote to PRO.
>
> Mainly because people are gonna do it anyway...
>
> Last thoughts on the thread, I got bigger fish to fry than array syntax
> sugar :D

PRO for me too. If we do this with automated tools we avoid the human
error. You have me convinced.

>
> -Chad
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Gabriel Wicke-3
In reply to this post by Chad
Overall I'm PRO, as consistency is worth a lot, and tools can apply
such changes consistently and efficiently.

We have applied broad formatting changes to large JS codebases using
jscs, which has worked well when those changes were well prepared.
Typically, this involved gradually refining the tool settings until a
reasonable diff was achieved.

On Fri, Feb 12, 2016 at 12:44 PM, Chad <[hidden email]> wrote:

> On Fri, Feb 12, 2016 at 9:14 AM Chad <[hidden email]> wrote:
>
>> On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler <[hidden email]>
>> wrote:
>>
>>> CON: don't do mass migration to new syntax, only start using new styles
>>> and
>>> features when touching the respective bit of code anyway. The argument is
>>> here
>>> that touching many lines of code, even if it's just for whitespace
>>> changes,
>>> causes merge conflicts when doing backports and when rebasing patches.
>>> E.g. if
>>> we touch half the files in the codebase to change to the new array
>>> syntax, who
>>> is going to manually rebase the couple of hundred patches we have open?
>>>
>>>
>>> As can be seen on the proposed patch I linked, several of the long term
>>> developers oppose mass changes like this. A quick round of feedback in the
>>> architecture committee draws a similar picture. However, perhaps there are
>>> compelling arguments for doing the mass migration that we haven't heard
>>> yet. So
>>> please give a quick PRO or CON, optionally with some rationale.
>>>
>>> My personal vote is CON. No rebase hell please! Changing to the syntax
>>> doesn't
>>> buy us anything.
>>>
>>>
>> CON, for all the reasons you mentioned. Also: style only changes are pain
>> when you're
>> trying to annotate/blame a particular line of code.
>>
>> ESPECIALLY for something so silly as array formatting which gains us
>> *absolutely nothing*
>>
>> -Chad
>>
>
> I change my vote to PRO.
>
> Mainly because people are gonna do it anyway...
>
> Last thoughts on the thread, I got bigger fish to fry than array syntax
> sugar :D
>
> -Chad
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l



--
Gabriel Wicke
Principal Engineer, Wikimedia Foundation

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Guillaume Lederrey
I'm not going to touch much PHP code any time soon, so feel free to ignore
my opinion.

I'm all for a consistent code base, or at least for a clear rule, even if a
bit arbitrary. Style issues are too often a matter of taste, so having a
clear and enforced rule allows us to stop the discussion and focus on
something more important.

Since we should have a rule, that rule should be as much as possible the
"state of the art". In this case it seems clear that new array syntax is a
winner.

Now that we have a rule, we need to enforce it (style check during CI).
This is a clear communication of the intent and again helps us stop wasting
time on manual reviews to enforce the rule. Code review time is precious
and should be spent on important stuff.

Last question: how do we manage the transition? Changing perfectly working
code just for the sake of style seems a bit like wasted resources. So we
should ensure that new code, or any code that is touched follows the new
rule, we ignore existing untouched code. We need the appropriate tooling to
make that possible, and that's why I need to find some time to deploy a
SonarQube instance as a proof of concept.


Sorry if I'm rambling, it's a bit late and my English is rusty... but I
always find discussions about code style interesting. Mostly because I want
us to stop having them ...



On Fri, Feb 12, 2016 at 9:54 PM, Gabriel Wicke <[hidden email]> wrote:

> Overall I'm PRO, as consistency is worth a lot, and tools can apply
> such changes consistently and efficiently.
>
> We have applied broad formatting changes to large JS codebases using
> jscs, which has worked well when those changes were well prepared.
> Typically, this involved gradually refining the tool settings until a
> reasonable diff was achieved.
>
> On Fri, Feb 12, 2016 at 12:44 PM, Chad <[hidden email]> wrote:
> > On Fri, Feb 12, 2016 at 9:14 AM Chad <[hidden email]> wrote:
> >
> >> On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler <[hidden email]>
> >> wrote:
> >>
> >>> CON: don't do mass migration to new syntax, only start using new styles
> >>> and
> >>> features when touching the respective bit of code anyway. The argument
> is
> >>> here
> >>> that touching many lines of code, even if it's just for whitespace
> >>> changes,
> >>> causes merge conflicts when doing backports and when rebasing patches.
> >>> E.g. if
> >>> we touch half the files in the codebase to change to the new array
> >>> syntax, who
> >>> is going to manually rebase the couple of hundred patches we have open?
> >>>
> >>>
> >>> As can be seen on the proposed patch I linked, several of the long term
> >>> developers oppose mass changes like this. A quick round of feedback in
> the
> >>> architecture committee draws a similar picture. However, perhaps there
> are
> >>> compelling arguments for doing the mass migration that we haven't heard
> >>> yet. So
> >>> please give a quick PRO or CON, optionally with some rationale.
> >>>
> >>> My personal vote is CON. No rebase hell please! Changing to the syntax
> >>> doesn't
> >>> buy us anything.
> >>>
> >>>
> >> CON, for all the reasons you mentioned. Also: style only changes are
> pain
> >> when you're
> >> trying to annotate/blame a particular line of code.
> >>
> >> ESPECIALLY for something so silly as array formatting which gains us
> >> *absolutely nothing*
> >>
> >> -Chad
> >>
> >
> > I change my vote to PRO.
> >
> > Mainly because people are gonna do it anyway...
> >
> > Last thoughts on the thread, I got bigger fish to fry than array syntax
> > sugar :D
> >
> > -Chad
> > _______________________________________________
> > Wikitech-l mailing list
> > [hidden email]
> > https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
>
> --
> Gabriel Wicke
> Principal Engineer, Wikimedia Foundation
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Bryan Davis
In reply to this post by Legoktm
On Fri, Feb 12, 2016 at 12:26 PM, Legoktm <[hidden email]> wrote:
> I think you're going to end up in rebase hell regardless, so we should
> rip off the bandaid quickly and get it over with, and use the automated
> tools we have to our advantage.
>
> So, if we're voting, I'm PRO.

+2

Bryan
--
Bryan Davis              Wikimedia Foundation    <[hidden email]>
[[m:User:BDavis_(WMF)]]  Sr Software Engineer            Boise, ID USA
irc: bd808                                        v:415.839.6885 x6855

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

bawolff
In reply to this post by Chad
> Last thoughts on the thread, I got bigger fish to fry than array syntax
> sugar :D
>
> -Chad

+1 to that.

--
bawolff

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Legoktm
In reply to this post by Daniel Kinzler
On 02/12/2016 07:27 AM, Daniel Kinzler wrote:
> Please give a quick PRO or CON response as a basis for discussion.

No one has responded in a few days, and the current count is 13-5-2, so
I'm going to find a time to do the mass migration when there aren't that
many people making core changes and do this today or tomorrow.

-- Legoktm

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Jon Robson-2
FYI for future reference Phabricator has a great poll feature that may be
useful for these kind of votes:
https://phabricator.wikimedia.org/vote/


On Tue, Feb 16, 2016 at 1:52 PM, Legoktm <[hidden email]>
wrote:

> On 02/12/2016 07:27 AM, Daniel Kinzler wrote:
> > Please give a quick PRO or CON response as a basis for discussion.
>
> No one has responded in a few days, and the current count is 13-5-2, so
> I'm going to find a time to do the mass migration when there aren't that
> many people making core changes and do this today or tomorrow.
>
> -- Legoktm
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Greg Grossmeier-2
<quote name="Jon Robson" date="2016-02-16" time="13:53:56 -0800">
> FYI for future reference Phabricator has a great poll feature that may be
> useful for these kind of votes:
> https://phabricator.wikimedia.org/vote/

See an example at:
https://phabricator.wikimedia.org/V7

--
| Greg Grossmeier            GPG: B2FA 27B1 F7EB D327 6B8E |
| identi.ca: @greg                A18D 1138 8E47 FAC8 1C7D |

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Mass migration to new syntax - PRO or CON?

Legoktm
In reply to this post by Legoktm
On 02/16/2016 01:52 PM, Legoktm wrote:
> On 02/12/2016 07:27 AM, Daniel Kinzler wrote:
>> Please give a quick PRO or CON response as a basis for discussion.
>
> No one has responded in a few days, and the current count is 13-5-2, so
> I'm going to find a time to do the mass migration when there aren't that
> many people making core changes and do this today or tomorrow.

{{done}}.

-- Legoktm

_______________________________________________
Wikitech-l mailing list
[hidden email]
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
12