Skip to content

Validate and standardize exposure of command classes#18

Merged
gigabo merged 3 commits intoasini:masterfrom
gigabo:command-validation
Sep 23, 2016
Merged

Validate and standardize exposure of command classes#18
gigabo merged 3 commits intoasini:masterfrom
gigabo:command-validation

Conversation

@gigabo
Copy link
Copy Markdown
Contributor

@gigabo gigabo commented Sep 21, 2016

Groundwork for pluggable commands.

Ensure that command classes:

  • Are named *Command
  • Are uniquely named
  • Extend the Command base class

Groundwork for pluggable commands.

Ensure that command classes:

- Are named `*Command`
- Are uniquely named
- Extend the `Command` base class
Comment thread src/Command.js Outdated
}

export function commandNameFromClassName(className) {
return className.replace("Command", "").toLowerCase();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we have CommandCommand? 😄

return className.replace(/Command$/, '').toLowerCase();?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Will update.

Comment thread src/Command.js Outdated
return className.replace("Command", "").toLowerCase();
}

export function exposeCommands(obj, commands) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if instead of mutating the argument, we just return the exposed commands?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's probably nicer for now. I had been anticipating a plugin API that added commands after-the-fact, but we can always change the API here if we need to for that.

@gigabo
Copy link
Copy Markdown
Contributor Author

gigabo commented Sep 23, 2016

I think I've addressed all of the feedback here. Will merge this afternoon barring objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants