Topic on Manual talk:Coding conventions/Flow

The convention currently states: Do not put spaces in brackets when accessing array elements. However, this is awkward if the index is not a literal, especially if it is an expression that contains spaces. For example:

  $a[0]
  $a['foo']
  $a[$i]
  $a[$i + 1]
  $a['x' . $name]
  $a[$this->get( $something, foo)]

We seem to already deviate from the convention in such cases, I found more than 100 places in core that use spaces in array indexes. I propose to change the convention as follows: Do not put spaces in brackets when accessing array elements if the index is a literal or variable. Do put spaces after the opening bracket and before the closing bracket if the index is an expression that by convention should contain spaces anywhere. For example:

  $a[0]
  $a['foo']
  $a[$i]
  $a[ $i + 1 ]
  $a[ 'x' . $name ]
  $a[ $this->get( $something, foo) ]

I think that is much more readable and intuitive. What do you think?

I'd personally also be fine to always add spaces in array indexes, if that is preferable for consistency.

(I've noted in the title that you're talking about Manual:Coding conventions/PHP rather than Manual:Coding conventions because the talk pages were all merged together for visibility.)

I think that we should do this, and for a little bit further and just use spaces always in square brackets; this would align our PHP convention with our JS one, which would make the rule very simple to remember.

The old excuse that this would be a lot of work is not a real problem, as we now have both auto-fixers in the linters and an auto-upgrader bot that will fix every repo for us.

I think the concern with changes like that was less that it's a lot of work and more that it touches a lot of lines (and so is a pain in the ass when doing git blame).

Apparently GitHub supports a default ignore-revs file these days; not sure about other tools.

Apparently that feature was added to git blame in v2.23, so that would be a good way forward, agreed.

Agree about using spaces in declarations and accesses, i.e. [ 'foo', 'bar' ], $a[ 0 ], and []. Do not use different rules for declaration vs. access (or destructuring, etc.), nor for index being simple (literal or variable) or an expression.

It looks like our coding conventions on PHP and JS have diverged on the issue of spaces inside of brackets. Manual:Coding conventions/PHP#Spaces states we should not do $c = $a[ 0 ];, but Manual:Coding conventions/JavaScript#Spaces states that we should, and it is one of the rules and autofixes in eslint-config-wikimedia.

Should we perhaps pick one way of doing it, then update the other coding convention page and linter to also do it that way, for consistency?

That would mean mass changes to the codebase (arrays are pretty common). We generally try to avoid those for minor style issues because they cause a lot of merge conflicts, and make git blame harder to use.

We can add any particularly-noisy commits to a .git-blame-ignore-revs file if needed?

We can but jgit doesn't seem to support it so presumably gitiles won't either. (It would work locally as long as you have git 2.23+, or in Github.)

Hmm, yeah, that's irritating. Thanks for digging.

When doing a "blaming search", all tools I usually use (SmartGit, GitHub website, Gitiles) let go to the preceding change, thus allowing to pursue the search. Ok, one annoying step is still added, but it doesn't prevent at all to complete the search. For perspective, file moves are much, much more annoying!

So, for this case, and also more generally speaking, I don't think we should avoid doing changes because of Git-related considerations.

@Jdforrester (WMF) here's a test: Gerrit change 1037943

One thing to note is that you do need to configure blame.ignoreRevsFile manually, it has no default value.

@Tgr (WMF): That's great, thank you!

Back to the wider topic, now we have a mechanism to ignore noisy commits, do we want to proceed with this?