Page MenuHomePhabricator

Use mediawiki codesniffer v33 in actively developed Wikibase related repositories and retire wikibase-codesniffer
Closed, ResolvedPublic5 Estimated Story Points

Description

Context

In T253624 we looked at if we wanted to continue maintaining our own set of codesniffer sniffers and decided not to.
In T164653 we reviewed the rules that were in wikibase-codesniffer and moved many of them to mediawiki codesniffer.
Per T253624#6373219 the rules that needed to be moved in order to no longer need wikibase-codesniffer have been merged.
Now that v33 has been released, we should be able to remove wikibase-codesniffer entirely and retire the project (using rules with a slightly modified name)
The overhead that this will reduce is discussed slightly in T253624#6165346

My suggestion for this migration would be to copy-paste the entire file https://github.com/wmde/WikibaseCodeSniffer/blob/master/Wikibase/ruleset.xml to every codebase that currently uses the WikibaseCodeSniffer, and slowly simplify it, e.g. remove exclusions you don't want to exclude any more, as well as remove additions you don't care about. I suggest to keep the comments, as they help other developers to understand that some exclusions are very intentional, and the Wikidata team doesn't want anybody to re-enable them (if this is still the case). Note that excluding a sniff is not a bad thing, and doesn't mean the code is bad. At this point we have sooo many sniffs, and many of them are about teeny tiny details nobody would really argue about. Feel free to diverge from the MediaWiki rule set a bit if it makes your team feel more comfortable.

To avoid questionable effort of updating code style in repositories that are not actively developed, the update of library used is to be made in the actively developed Wikibase related repositories listed below:

Acceptance criteria:

  • The same rules are applied on actively developed Wikibase related repositories (see list below) that currently use wikibase-codesniffer after the change
  • The wikibase-codesniffer project is appropriately discontinued (on git, in README, in packagist etc)

List of repos to apply this change to:

  • gerrit extensions / EntitySchema -> T268826
  • gerrit extensions / Wikibase -> T268827
  • gerrit extensions / WikibaseCirrusSearch -> T268828
  • gerrit extensions / WikibaseLexeme -> T268829
  • gerrit extensions / WikibaseLexemeCirrusSearch -> T268830
  • gerrit extensions / WikibaseQualityConstraints -> T268831

DUring story time it was decided not to touch these as part of this ticket and only update them when next touched.

  • github wmde / WikibaseDataModel
  • github wmde / WikibaseDataModelSerialization
  • github wmde / WikibaseDataModelServices
  • github wmde / WikibaseInternalSerialization

Related Objects

Event Timeline

Addshore updated the task description. (Show Details)
Addshore added a subscriber: thiemowmde.

My suggestion for this migration would be to copy-paste the entire file https://github.com/wmde/WikibaseCodeSniffer/blob/master/Wikibase/ruleset.xml to every codebase that currently uses the WikibaseCodeSniffer, and slowly simplify it, e.g. remove exclusions you don't want to exclude any more, as well as remove additions you don't care about. I suggest to keep the comments, as they help other developers to understand that some exclusions are very intentional, and the Wikidata team doesn't want anybody to re-enable them (if this is still the case). Note that excluding a sniff is not a bad thing, and doesn't mean the code is bad. At this point we have sooo many sniffs, and many of them are about teeny tiny details nobody would really argue about. Feel free to diverge from the MediaWiki rule set a bit if it makes your team feel more comfortable.

DannyS712 renamed this task from Use mediawiki codesniffer v33 or v32.1 and retire wikibase-codesniffer to Use mediawiki codesniffer v33 and retire wikibase-codesniffer.Nov 2 2020, 9:40 PM
DannyS712 updated the task description. (Show Details)
WMDE-leszek renamed this task from Use mediawiki codesniffer v33 and retire wikibase-codesniffer to Use mediawiki codesniffer v33 in actively developed Wikibase related repositories and retire wikibase-codesniffer.Nov 26 2020, 12:04 PM
WMDE-leszek updated the task description. (Show Details)
WMDE-leszek set the point value for this task to 5.

Well, the first round of LibUp-powered phpcs updates is now on Gerrit, and I’m not exactly impressed… judging from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseQualityConstraints/+/646637, it doesn’t look like LibUp is currently written in such a way as to respect permanent, deliberate exclusions from the ruleset; rather, the intention seems to be that all projects must adhere to the full ruleset, and any exclusions are only temporary because they can’t be autofixed yet – as soon as a human fixes them, or they become autofixable, the exclusion is removed from the config.

Well, the first round of LibUp-powered phpcs updates is now on Gerrit, and I’m not exactly impressed… judging from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseQualityConstraints/+/646637, it doesn’t look like LibUp is currently written in such a way as to respect permanent, deliberate exclusions from the ruleset; rather, the intention seems to be that all projects must adhere to the full ruleset, and any exclusions are only temporary because they can’t be autofixed yet – as soon as a human fixes them, or they become autofixable, the exclusion is removed from the config.

Yes. We call them the Wikimedia coding standards. :-)

Well, the first round of LibUp-powered phpcs updates is now on Gerrit, and I’m not exactly impressed… judging from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseQualityConstraints/+/646637, it doesn’t look like LibUp is currently written in such a way as to respect permanent, deliberate exclusions from the ruleset; rather, the intention seems to be that all projects must adhere to the full ruleset, and any exclusions are only temporary because they can’t be autofixed yet – as soon as a human fixes them, or they become autofixable, the exclusion is removed from the config.

For those most part that's how it's generally worked. libup works by swapping in a blank config with no rules disabled, autofixing whatever is possible, then restoring the old config and removing whatever now passes. We'll need to have it respect <severity>0</severity> as the explicit way to disable something that isn't supposed to be fixed.

Everything on gerrit is done, some repos on github seems still to use it: https://codesearch.wmcloud.org/search/?q=wikibase-codesniffer&i=nope&files=&repos=

Addshore updated the task description. (Show Details)
WMDE-leszek added a subscriber: WMDE-leszek.

This does not seem completed in a way WMDE wanted it. Please let us finish it and close it when we're done.

I feel like we need to talk about this a little more.

We call them the Wikimedia coding standards.

Yes, and no. Sure, one of the purposes of the MediaWiki-Codesniffer is to encode https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP. But the two are not entirely identical. The manual page lists things that aren't or just can't be checked by a sniff (for example, it's probably impossible to tell if a == should be a === or vice versa). And we have more and more sniffs for details that are not listed on the manual page. For example, the manual page doesn't say anything about assertCount and such – yet we have sniffs that discourage using certain combinations.

That alone is not a problem.

We typically think of a coding standard as something that should ideally apply to 100% of all code. When code doesn't conform, it should probably be rewritten. But here is the problem: We started to add more and more sniffs with known exceptions that are not "broken" and don't need "fixing". Other sniffs are disputable. For example, I think that the extra space in @param bool $enable really, really isn't a problem (this happens when you vertically align multiple @param). It should be ok when the owner of a codebase excludes this. T264948 is a another example that already caused issues, see T269332. Is this a problem? Shouldn't be. We have know solutions, e.g. using // phpcs:… comments or <severity>, as discussed above. But it can become an issue when a sniff's message or a tool like LibUp create the impression that the owner of a codebase is doing something wrong – when they don't. It's about the tone. That's why I hope we can do more fine-tuning like this.