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

fix: fix aliases in config.commandIDs missing the default topic separator #1229

Conversation

myarmolinsky
Copy link
Contributor

@myarmolinsky myarmolinsky commented Oct 22, 2024

This fixes the following issue/example:

  • cli add topic3 is an alias for cli set topic3
  • cli add is an alias for cli set
  • Try to pipe content to the alias command: cat foo | cli add topic3
  • The content of foo should be sent to cli add topic3, but it is instead sent to cli add
  • In contrary, cat foo | cli set topic3 works fine: the content of foo is sent to cli set topic3 rather than cli set

Please let me know if I am missing some context, or if there is a better spot to apply this change. I was looking for a spot to apply it before Config.load is called (or at least earlier on than when loadPluginsAndCommands is called in Config.load) but I couldn't find an appropriate spot

@myarmolinsky myarmolinsky force-pushed the fix-aliases-missing-default-topic-separator-in-config-commands-list branch 2 times, most recently from 3f5ff6f to 53f3743 Compare October 22, 2024 00:16
@mdonnalley
Copy link
Contributor

@myarmolinsky Thanks for taking the time to make the PR!

Quick question: how are you defining the aliases? I found that I'm only able to replicate the issue if I set the aliases like this: aliases = ['add topic3'] (space separated). If set it to aliases = ['add:topic3'] (colon separated) - it works

We never intended for that aliases array to be space separated - although I can't think of a good reason not to support it

Copy link
Contributor

@mdonnalley mdonnalley left a comment

Choose a reason for hiding this comment

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

Looks good!

Just need one change then I'll merge into a separate feature branch so that all the integration tests can run. Once those pass, I'll merge to main

@@ -780,23 +780,27 @@ export class Config implements IConfig {
}

const handleAlias = (alias: string, hidden = false) => {
if (this._commands.has(alias)) {
const aliasWithDefaultTopicSeparator = alias.replaceAll(' ', ':')
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a util function for whenever we need to do this:

import { toStandardizedId } from '../util/ids'
const aliasWithDefaultTopicSeparator = toStandardizedId(alias)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 I should have noticed this, my bad

@mdonnalley mdonnalley added the enhancement New feature or request label Oct 22, 2024
Copy link

git2gus bot commented Oct 22, 2024

This issue has been linked to a new work item: W-17039093

@myarmolinsky
Copy link
Contributor Author

myarmolinsky commented Oct 22, 2024

Thank you for always being so quick to review!

I am indeed setting the aliases as aliases = ['add topic3']. We use as a topic separator instead of : in the balena CLI. In this particular case we had a command balena key add which I created an alias for as I intend to deprecate the old one. The new command name is balena ssh-key add and I made the old one an alias for the new one. Given that the old command had the space when we would use it (as well as in any help/warning output users would receive, including the deprecated command warning as I had committed previously), my instinct was to add it to the aliases array with the rather than with a :.

I have just tested aliases = [add:topic3] instead of with the space and I see that it indeed still appears to print the correct expected output help/warnings, which is great to see as well. In general I suppose the ideal UX for us would be for everything to respect/expect the topic separator of choice (hence this PR and the previous one).

@myarmolinsky myarmolinsky force-pushed the fix-aliases-missing-default-topic-separator-in-config-commands-list branch from 53f3743 to 0eca8b3 Compare October 22, 2024 15:21
@myarmolinsky myarmolinsky force-pushed the fix-aliases-missing-default-topic-separator-in-config-commands-list branch from 0eca8b3 to bbd8186 Compare October 22, 2024 15:23
@mdonnalley mdonnalley changed the base branch from main to mdonnalley/1229 October 22, 2024 15:29
@mdonnalley mdonnalley merged commit 9671934 into oclif:mdonnalley/1229 Oct 22, 2024
1 check passed
mdonnalley added a commit that referenced this pull request Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants