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

Added warning when a command with the same name is loaded multiple times #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nahilep
Copy link
Contributor

@nahilep nahilep commented Feb 23, 2019

With this PR there will be a warning when a command with the same name will be loaded again.
There is room for improvement:

  1. should we also check aliases?
  2. Please help me with the warning message I put in there. Should it be different?

@shawncplus
Copy link
Member

  1. Yeah, checking aliases would be good since aliases currently bypass this check
  2. After the Logger.warn you should continue
  3. It would help if you mentioned which bundle the command came from, e.g., "Command '${commandName}' (${bundle} bundle) already exists and will be discarded`

@markscho
Copy link
Contributor

I would go a step further in how verbose you're going to be:
Existing command '${commandName}' from bundle [${this.state.CommandManager.get(commandName).bundle}] replaced by one from [${bundle}].

@nahilep
Copy link
Contributor Author

nahilep commented Feb 23, 2019

I did not continue after the warning, so the behavior doesn't change in case we have some ranviers out there with duplicate commands.

@shawncplus shawncplus added feedback PR/Issue has pending feedback and removed feedback PR/Issue has pending feedback labels Feb 24, 2019
@shawncplus shawncplus added this to the 3.1 milestone Aug 22, 2019
@shawncplus shawncplus added feedback PR/Issue has pending feedback and removed feedback PR/Issue has pending feedback labels Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants