Skip to content

Friendly command names in help text#99

Open
JunhaoSLi wants to merge 6 commits intodev-masterfrom
friendly-command-names-in-help-text
Open

Friendly command names in help text#99
JunhaoSLi wants to merge 6 commits intodev-masterfrom
friendly-command-names-in-help-text

Conversation

@JunhaoSLi
Copy link
Copy Markdown
Collaborator

@JunhaoSLi JunhaoSLi commented Feb 13, 2022

Draft PR with a single commit to show how I'm planning on changing the config to do 'friendly names' of commands from the config. Will convert to full PR once bot.js changes are finished and tested.

"alias": "Cadence play "
}
],
"commandFriendlyNames": {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should we merge this with commandDescriptions?

ie

"commandDescriptions": {
    "play": {
      "description": "Starts playing Cadence in your voice channel", 
      "friendly": "play" 
    },
    ... 
} 

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh yes, good idea

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I suppose if we do that it should be clear and say friendlyName or something though, shouldn't it...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, friendlyName is probably best. I'll make it that.

}
],
"commandFriendlyNames": {
"play": "play",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just a thought - Is it worse to pollute config with these friendly names that match the key, or... Maybe we could default the friendly name to the key if it's not present?

Hm, that seems harder than it's worth. But just a thought...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I think it'll be easy enough on the code side and intuitive enough from the config side so I'll add it then unless you decide against it

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If you think a default isnt too much trouble I'm a fan. Just don't invest too much effort into it, I'm not demanding you add it.

"It can be used in one of two ways:",
" 1. Without any argument (`%help`), it will generate a summary of commands and a brief snippet of their purpose.",
" 2. Provided the name of a command (`%helpTopic help`), it will provide details regarding the specific usage of that command."
" 1. Without any argument (`%friendly:help`), it will generate a summary of commands and a brief snippet of their purpose.",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This begs the question of when we should use friendly name vs canonical (an invocation)

So here, are we saying without any argument as in help, or are we saying "if you run it without any argument by sending the message Cadence help"?

Adding the friendly name creates the writing style concern of when you write a friendly name to refer to the command, and when you write the invocation of the command - For example, I might write "%friendly:search results will be saved for a future use of %request to select a song."

Not saying this is necessarily wrong, mind you - but we should try to avoid letting ourselves mindlessly use one form or the other, now that you're giving us the freedom of using either.

Copy link
Copy Markdown
Collaborator Author

@JunhaoSLi JunhaoSLi Feb 13, 2022

Choose a reason for hiding this comment

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

Oh yeah, I wasn't thinking here and blindly changed all the canonical names into friendly names. It seems these should all be left as canonical names except the use in nowplaying command's help text

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That sounds good to me!

@JunhaoSLi JunhaoSLi marked this pull request as ready for review February 13, 2022 23:18
Copy link
Copy Markdown
Owner

@za419 za419 left a comment

Choose a reason for hiding this comment

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

I still have some questions about the details, but I have no strong objections to merging.

);
return;
} else if (targetTopicAliases.includes(detailsObject.alias)) {
targetTopicAliases.push(detailsObject.alias)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why are we pushing this onto the array that already contains it before returning?

Copy link
Copy Markdown
Collaborator Author

@JunhaoSLi JunhaoSLi Apr 19, 2022

Choose a reason for hiding this comment

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

Oh yes, it's just for the join in the warning log which gets printed below, e.g. "Help topic nowplaying is part of an alias loop: nowplaying > nowplaying"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I can add a comment to make this clearer?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah, I see. Yeah, I'd usually expect that to be a '+= > ${detailsObject.alias} sort of expression...

A comment is fine. Thanks

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.

2 participants