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

Adds help messages to the help command #89

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

andreban
Copy link
Member

Closes #87

@andreban andreban requested a review from PEConn January 29, 2020 14:22
const HELP_MESSAGES = new Map<string, string>(
[
['main', [
'llama-pack [command] <options>',
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use template literals for multiline strings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(if you want, I'm also fine with this way.)

Copy link
Member Author

@andreban andreban Jan 29, 2020

Choose a reason for hiding this comment

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

Tried it out, but it kind of breaks the indentation, as otherwise the blank characters at the beginning of the line are printed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah, I'd have it formatted something like:

const HELP_MESSAGES = new Map<string, string> (
    [
      ['main',
`
llama-pack [command] <options>

build ............... generates an Android APK from a TWA Project
help ................ shows this menu
`],
      ['init',
`
...
`

But yeah, I'm fine with your way. (Mixing the indent of code and output strings is usually horrible - I guess the "proper" way to do this would be to have the help strings nicely formatted in text files that we read in (and it would allow for easier internationalisation, if we ever get that far!).


// Should never happen, as we know we have a message for main,
// but TypeScript doesn't know it.
if (message === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, so what you want here is an ! (a (Non-null assertion operator)[https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#non-null-assertion-operator]).

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, the linter configuration dislikes it:

 69:12  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion

Copy link
Member Author

@andreban andreban Jan 29, 2020

Choose a reason for hiding this comment

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

Here's the rule: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-non-null-assertion.md

I can keep the current role and the rule or add an eslint-ignore-line. I kind of prefer the first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree with that rule - the point of strict null checking is for the compiler to catch things for you that you would have missed. In this case, we've acknowledged the possibility of null and said that we want to crash if that's the case.

Sorry, I don't understand what you're saying with "I can keep the current role and the rule"

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Changed eslint.json to remove the rule and removed the unnecessary check from help.ts.

@andreban andreban changed the title Adds help messages to the help command … Adds help messages to the help command Jan 29, 2020
- Cleans-up code on `help.ts` that was affected by the rule.
@andreban andreban requested a review from PEConn January 30, 2020 11:09
@andreban andreban merged commit 2016cac into GoogleChromeLabs:master Jan 30, 2020
@andreban andreban added this to the 1.0.0 milestone Feb 3, 2020
@andreban andreban deleted the help_messages branch February 13, 2020 08:14
@andreban andreban added the enhancement New feature or request label Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the help command helpful
3 participants