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).
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.
-- daniel
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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
On Fri, Feb 12, 2016 at 9:14 AM, Chad innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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*
Agree on many levels. Please, let's focus on solving problems and improving how our code works rather than how our code looks.
-Chad _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
First, a thank you for Daniel for writing such a balanced, informative email on a somewhat contentious subject!
As someone who will probably never touch a line of MW code, I'm not going to actually vote. But I do have experience in large, complex codebases and I'm going to argue for PRO.
We need to acknowledge that our codebase is going to change over time and if we aren't explicit and accepting of this reality, we're not going to be able to manage it well. Consider future developers who may only ever know new syntax and their desire and ability to work on a codebase that doesn't reflect new techniques and syntax in its implementation.
-Toby
On Fri, Feb 12, 2016 at 9:44 AM, Purodha Blissenbach < purodha@blissenbach.org> wrote:
CON, for all the reasons mentioned.
Purodha
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Fri, Feb 12, 2016 at 9:14 AM Chad innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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
On 12 Feb 2016 12:44 p.m., "Chad" innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 9:14 AM Chad innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 9:14 AM Chad innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 gwicke@wikimedia.org 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 innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 9:14 AM Chad innocentkiller@gmail.com wrote:
On Fri, Feb 12, 2016 at 7:27 AM Daniel Kinzler daniel@brightbyte.de 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-- Gabriel Wicke Principal Engineer, Wikimedia Foundation
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
I also think we shouldn't mass migrate.
New code should use the new syntax and old code can be converted during larger refactors or similar things (when it is being touched anyway), but we shouldn't have "update syntax" only patches.
Cheers,
Marius
On 12.02.2016 16:27, 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).
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.
-- daniel
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi!
Please give a quick PRO or CON response as a basis for discussion.
My opinion: if it ain't broke, don't fix it. It's ok to use new syntax in new code, but spending time on changing perfectly working code just to use new array syntax looks like misplaced effort to me.
There are new features that I would have more support immediate change, like $this in closures - that makes code much more readable and less bug-prone. Even then, I'm ambivalent whether we need to touch the code that much, but I see a point of cleaning it up. But with array syntax, it's just a different syntax, and I don't see a lot of reason to mess with existing working code just to use it.
On Fri, Feb 12, 2016 at 7:27 AM, Daniel Kinzler daniel@brightbyte.de 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.
PRO. These syntax changes were implemented in PHP at the cost of breaking backward-compatibility, which tells you that people understood their value and were willing to pay a cost for modernizing and simplifying the language. If PHP was willing to pay it, why wouldn't we?
Hi!
PRO. These syntax changes were implemented in PHP at the cost of breaking backward-compatibility, which tells you that people understood their value
Wait, are we talking about the same thing? New array syntax does not break BC. Or you mean that if we use new array syntax, we'd break BC with older PHP versions? I'm not sure I understand your argument here.
On Fri, Feb 12, 2016 at 10:26 AM, Stas Malyshev smalyshev@wikimedia.org wrote:
Hi!
PRO. These syntax changes were implemented in PHP at the cost of breaking backward-compatibility, which tells you that people understood their
value
Wait, are we talking about the same thing? New array syntax does not break BC. Or you mean that if we use new array syntax, we'd break BC with older PHP versions? I'm not sure I understand your argument here.
I'm also a little puzzled. I thought it was a given we are embracing and modernizing newer PHP versions.
The question as I understood it, is should we touch every piece of our codebase in one big mega patch or update it gradually as and when we visit bits of the codebase (I get the impression the latter isn't happening due to a desire to have mega patches).
-- Stas Malyshev smalyshev@wikimedia.org
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi!
The question as I understood it, is should we touch every piece of our codebase in one big mega patch or update it gradually as and when we visit bits of the codebase (I get the impression the latter isn't happening due to a desire to have mega patches).
Same here and just to be clear, I'm for the gradual approach.
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.
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
PRO from me, for all the reasons mentioned by legoktm
On 12 February 2016 at 19:26, Legoktm legoktm.wikipedia@gmail.com wrote:
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.
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
PRO from me too.
Doing it gradually is just going to make the codebase inconsistent, and tooling can help point patches to the old style to migrate to the new one.
I'd rather do it quickly than have the inconsistency bleed through months or years.
On Fri, Feb 12, 2016 at 8:28 PM, Alex Monk krenair@gmail.com wrote:
PRO from me, for all the reasons mentioned by legoktm
On 12 February 2016 at 19:26, Legoktm legoktm.wikipedia@gmail.com wrote:
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.
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
I think it's also pertinent to note that we are finally reaching a state where PHP-CS can be voting, which was only achieved after a lot of hard work to make our codebase consistent.
If we make these changes gradually, we're basically throwing away all of the work that was just done recently.
Regards, -- Tyler Romeo https://parent5446.nyc 0x405D34A7C86B42DF
From: Joaquin Oltra Hernandez jhernandez@wikimedia.org Reply: Wikimedia developers wikitech-l@lists.wikimedia.org Date: February 12, 2016 at 14:31:54 To: Wikimedia developers wikitech-l@lists.wikimedia.org Subject: Re: [Wikitech-l] Mass migration to new syntax - PRO or CON?
PRO from me too.
Doing it gradually is just going to make the codebase inconsistent, and tooling can help point patches to the old style to migrate to the new one.
I'd rather do it quickly than have the inconsistency bleed through months or years.
On Fri, Feb 12, 2016 at 8:28 PM, Alex Monk krenair@gmail.com wrote:
PRO from me, for all the reasons mentioned by legoktm
On 12 February 2016 at 19:26, Legoktm legoktm.wikipedia@gmail.com wrote:
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.
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
PRO: preserving the blame history is a false hope because we liberally move files around, extract classes to separate files and so on. In other words, we do the usual refactoring work and in process we break blame at all times. Syntax updates are just another refactoring, nothing principally new. And no, when hell freezes over are inconsistent code standards better: we already have a code base inconsistent enough after 15 years of development and MUST avoid creating more.
On Fri, Feb 12, 2016 at 11:28 AM, Alex Monk krenair@gmail.com wrote:
PRO from me, for all the reasons mentioned by legoktm
On 12 February 2016 at 19:26, Legoktm legoktm.wikipedia@gmail.com wrote:
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.
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Fri, Feb 12, 2016 at 2:26 PM, Legoktm legoktm.wikipedia@gmail.com 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.
This. Just get it over with, and then it's only one patch screwing up rebases and blames instead of lots.
Although I haven't touched MediaWiki code for a year or so, based on my experience with large codebases with tons of contributors, I would be very much PRO.
I understand it is a pain, but as Legoktm points out, it is a manageable pain. Having a consistent and higher-quality code base is worth the migration pain. Three more advantages: * future changesets are cleaner, as one does not have to do the clean up in addition to the actual change they wanted to do * automatic testing tools can capture issues with a higher confidence if it doesn't have to take historical exceptions into account * most developers code by copy-and-paste of style, structures, and ideas. So even if a new styleguide is in place, it can often be the case that a developer will start building off the old styleguide as they simply keep their code consistent with the code that they are looking at
hth
On Fri, Feb 12, 2016 at 11:57 AM Brad Jorsch (Anomie) bjorsch@wikimedia.org wrote:
On Fri, Feb 12, 2016 at 2:26 PM, Legoktm legoktm.wikipedia@gmail.com 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.
This. Just get it over with, and then it's only one patch screwing up rebases and blames instead of lots.
-- Brad Jorsch (Anomie) Senior Software Engineer Wikimedia Foundation _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Fri, Feb 12, 2016 at 12:26 PM, Legoktm legoktm.wikipedia@gmail.com 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
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
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 legoktm.wikipedia@gmail.com 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 Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
<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
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@lists.wikimedia.org