Page MenuHomePhabricator

Allow namespace aliases in FullQualifiedClassNameSniff
Open, Needs TriagePublic

Description

In T308718 I am introducing a pattern in which MediaWiki\ResourceLoader is imported as the alias RL, so that short and ambiguous class names like ResourceLoaderContext can have their prefix stripped, becoming RL\Context at some call sites. The abbreviation RL is always used instead of ResourceLoader to avoid conflicting with the class of that name.

A few extensions have MediaWiki.Classes.FullQualifiedClassName enabled, and this sniff fails with RL\Context and similar references. See VisualEditor 793616 for example. But I think it is within the spirit of the rule to allow such an alias.

Possible implementations:

  • Replace it with a metric that measures the number of components in a class name reference, and limit it to 2. So the existing exception examples \Title and \Wikimedia\suppressWarnings() would continue to be allowed on the basis that they have 1 and 2 components, and RL\Context would because it has two components, whereas MediaWiki\ResourceLoader\Context would not be allowed because it has 3 components. Although this would allow MediaWiki\MediaWikiServices which is not intended.
  • Parse alias declarations, and allow the aliases which were seen in the file.

Related Objects

Event Timeline

I could also imagine adding a configurable property to the rule to list allowed qualified name prefix, so that you could allowlist RL\ and other aliases or namespaces where it makes sense.

One of the most important things I learned when I started working on code that is meant to be maintained by more than one person is to be specific, and avoid abbreviations at pretty much all costs. It's not worth it. Typing speed is not an argument any more in 2022, nor is screen space. I have an IDE with clever auto-completion. Even when I type super-cryptic shortcuts like "RLC" the IDE correctly guesses ResourceLoaderContext (or MediaWiki\ResourceLoader\Context in the new naming scheme).

Because of this I'm rather uncomfortable with the suggested style. What's the benefit of doing it like this?

use MediaWiki\ResourceLoader as RL;

class VisualEditorDataModule extends RL\Module {
    public function getScript( RL\Context $context ) {
    }
}

Why not do it as we always do?

use MediaWiki\ResourceLoader\Context;
use MediaWiki\ResourceLoader\Module;

class VisualEditorDataModule extends Module {
    public function getScript( Context $context ) {
    }
}

Or like this in case we are dealing with more complex code where generic terms like "context" would cause confusion.

use MediaWiki\ResourceLoader\Context as ResourceLoaderContext;
use MediaWiki\ResourceLoader\Module as ResourceLoaderModule;

class VisualEditorDataModule extends ResourceLoaderModule {
    public function getScript( ResourceLoaderContext $context ) {
    }
}

Or even simpler, stick to something that's not so overly generic in the first place.

use MediaWiki\ResourceLoader\ResourceLoaderContext;
use MediaWiki\ResourceLoader\ResourceLoaderModule;

class VisualEditorDataModule extends ResourceLoaderModule {
    public function getScript( ResourceLoaderContext $context ) {
    }
}

All this works with the sniff.