Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break cycle between Command and CommandMap (aliases, label) #6508

Open
dktapps opened this issue Nov 16, 2024 · 0 comments
Open

Break cycle between Command and CommandMap (aliases, label) #6508

dktapps opened this issue Nov 16, 2024 · 0 comments
Labels
BC break Breaks API compatibility Category: Core Related to internal functionality Easy task Probably really easy to do, good task for first-time contributors Type: Cleanup Removes or deprecates API methods or behaviour
Milestone

Comments

@dktapps
Copy link
Member

dktapps commented Nov 16, 2024

Description

Have CommandMap track commands' active aliases and "label" (primary alias) instead of Command

Justification

Since a command's active aliases are tracked by Command internally, a Command needs to ensure it is only registered to a single CommandMap at a time.

This is a dumb limitation and requires extra complexity. We could just have the active aliases tracked by CommandMap itself, since they're only actually important to the CommandMap anyway.

Going further, aliases could be entirely removed from Command. They could just be passed as a string[] to CommandMap->register().

In addition, if #6506 were implemented, Command->getName() (primary alias) could be deleted too, as its only actual use is for onCommand().

public function setLabel(string $name) : bool{
$this->nextLabel = $name;
if(!$this->isRegistered()){
$this->label = $name;
return true;
}
return false;
}
/**
* Registers the command into a Command map
*/
public function register(CommandMap $commandMap) : bool{
if($this->allowChangesFrom($commandMap)){
$this->commandMap = $commandMap;
return true;
}
return false;
}
public function unregister(CommandMap $commandMap) : bool{
if($this->allowChangesFrom($commandMap)){
$this->commandMap = null;
$this->activeAliases = $this->aliases;
$this->label = $this->nextLabel;
return true;
}
return false;
}
private function allowChangesFrom(CommandMap $commandMap) : bool{
return $this->commandMap === null || $this->commandMap === $commandMap;
}
public function isRegistered() : bool{
return $this->commandMap !== null;
}

public function setAliases(array $aliases) : void{
$this->aliases = $aliases;
if(!$this->isRegistered()){
$this->activeAliases = $aliases;
}
}

@dktapps dktapps added Category: Core Related to internal functionality BC break Breaks API compatibility Type: Cleanup Removes or deprecates API methods or behaviour labels Nov 16, 2024
@dktapps dktapps moved this to Todo in Commands rework Nov 16, 2024
@dktapps dktapps added this to the 6.0 milestone Nov 16, 2024
@dktapps dktapps added the Easy task Probably really easy to do, good task for first-time contributors label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Breaks API compatibility Category: Core Related to internal functionality Easy task Probably really easy to do, good task for first-time contributors Type: Cleanup Removes or deprecates API methods or behaviour
Projects
Status: Todo
Development

No branches or pull requests

1 participant