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

feat: improve help message for new command #320

Closed
wants to merge 18 commits into from

Conversation

lakshz
Copy link

@lakshz lakshz commented Jul 11, 2022

Description

  • Improved the help message of 'new' command with examples

Related issue(s)

Fixes #162

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@lakshz lakshz changed the title fix: Improved help message of 'new' command fix: improved help message of 'new' command Jul 11, 2022
@@ -15,14 +15,22 @@ export default class New extends Command {
static flags = {
help: Flags.help({ char: 'h' }),
'file-name': Flags.string({ char: 'n', description: 'name of the file' }),
example: Flags.string({ char: 'e', description: 'name of the example to use' }),
example: Flags.string({ char: 'e', description: `name of the example to use. Available examples are: \n\t- default-example.yml \n\t- somethingElseWithSpecificProtocol.json (MQTT)` }),
Copy link
Member

Choose a reason for hiding this comment

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

hey, sorry, I was not super clear in related github issue.

The point here is that there are multiple different examples, and user of CLI has no idea about them. In my issue I just wanted to indicate how list of examples was suppose to look like. But in fact, list is much larger and should be built in a dynamic way in the help, not hardcoded.

when you run npm run build you will notice that in /assets/examples directory there is much more. And there is a file called examples.json. We already read this file -> https:/asyncapi/cli/pull/320/files#diff-bddcbb9edc64eedc14d10908f9c6a72353ea7a0cea813b34edc6d163f2a33f92R76 so we can list examples in the interactive mode of asyncapi new. Anyway, we should definitely use it to list all the examples in the help, so folks know what they can actually use.

I hope it makes sense 😄

Copy link
Author

@lakshz lakshz Jul 12, 2022

Choose a reason for hiding this comment

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

Okay, got it, thanks. Working on it.

@@ -24,6 +46,10 @@ export default class New extends Command {
static args = []

async run() {
const exampleDescription = await getExamplesFlagDescription();
this.flags.example = Flags.string({ char: 'e', description: exampleDescription});
Copy link
Author

Choose a reason for hiding this comment

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

Okay, so it's been a while now that I'm stuck on this.
The problem that I'm facing is that I fetched the example.json, mapped it, and made the description out of it. But, how do I add to my static flags?
I've tried to add it inside the run() function, before parsing the New class, but it's not working.

Copy link
Member

Choose a reason for hiding this comment

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

I think flag building doesn't like async/await and promises 😄

when I quickly play with your code:

  • replace any async/await with normal functions and switch to readFileSync
  • fix building description like this:
    let description = 'name of the example to use. Available examples are:';
    
    for (const element of examples) {
      description += (`\n\t - ${element.value}`);
    }
    
    

it works as expected:
Screenshot 2022-07-19 at 14 17 01

full code:

function loadExampleFile() : Example[] {
  const exampleFiles = readFileSync(resolve(__dirname, '../../assets/examples/examples.json'), { encoding: 'utf8' });
  //console.log(JSON.parse(exampleFiles));
  return JSON.parse(exampleFiles);
}

function getExamplesFlagDescription() : string {
  const examples = loadExampleFile();
  let description = 'name of the example to use. Available examples are:';
 
  for (const element of examples) {
    description += (`\n\t - ${element.value}`);
  }

  return description;
}
    example: Flags.string({ char: 'e', description: getExamplesFlagDescription()}),

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks a lot

@lakshz lakshz requested a review from derberg July 14, 2022 22:36
@derberg
Copy link
Member

derberg commented Jul 21, 2022

@LakshyaSatpal looking good man! Please just add a unit test that validates if list of examples is rendered? I'm just pretty sure that in few months someone will say "why do we use sync function here, lemme refactor" 😆

in the test, please do not "hardcode" list of examples as the tests will be affected by changes in examples list. So in test also read examples dynamically

@lakshz
Copy link
Author

lakshz commented Jul 29, 2022

Hi @derberg !
I was going through oclif documentation for testing for commands. I'm finding it challenging to write a test to check if a file is rendered.
I mean, I couldn't get what do I have to actually test, validate if the user entered the correct example name from available examples? or only check the working of loadExampleFile synchronous function that we wrote?
Btw, this is the first time I am writing unit tests. Please bear with me 😅

@derberg
Copy link
Member

derberg commented Jul 29, 2022

@LakshyaSatpal no worries. Happy to help you if you need.

  • you need to provide test for new functionality. New functionality is: much better help message with list of examples. We need to have a test, that will evaluate if the list of examples from /assets/examples is included in the help message that you just created.
  • have a look on existing tests https:/asyncapi/cli/tree/master/test/commands for commands. See how we check if certain message appears in the console

I recommend, especially if it is your first time, first create a test, where you basically hardcode the expected message in the text, so you check if the message printed in terminal match the message that you hardcode in test (with list of examples).

Once ☝🏼 works, then work on making sure that the list of examples is not hardcoded, but you read in dynamically inside the test, from examples dir

@lakshz
Copy link
Author

lakshz commented Jul 29, 2022

@derberg I wrote this simple test in the new.test.ts file where I checked for help message to contain the first example which I hard coded.

test
		.stdout()
		.command(['new', '--help'])
		.it('help includes available examples', (ctx, done) => {
			expect(ctx.stdout).to.contain('- simple.yml');
			done();
		});

This shows me an error of AssertionError: expected '' to include '- simple.yml'. I believe, it is not getting that I'm checking for help flag command. Am I moving in the right direction?

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication


test
.stdout()
.command(['new', '-h'])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.command(['new', '-h'])
.command(['new', '--help'])

try doing this

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I tried, getting the same error

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not working only in the test environment 🤔

Copy link
Member

Choose a reason for hiding this comment

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

found this though oclif/oclif#142 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

the reason is that actually new --help in test is not successful and throws:

TypeError: cmd._help is not a function

I get the same when I try bin/run new -h with CLI. But bin/run new --help works fine when I try your branch 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@Souvikns maybe we should just resign from tests in this PR and write an issue about what I described above and get that solved first. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

ok, we need to go forward here:
@LakshyaSatpal please look at other comments, address them, and remove the test. There is no way to make it run now. The main functionality will have to stay untested 😄

also please add "additionalHelpFlags": ["-h"], to oclif configuration in package.json (line 97) so we at least make -h to work too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's not add tests in this PR, but open an issue about how we cannot test the help messages. And once oclif fixes it we can add help tests for all other commands.

@derberg
Copy link
Member

derberg commented Aug 16, 2022

@LakshyaSatpal hey, you need a hand? I was out on holidays, but now back 💪🏼

@lakshz
Copy link
Author

lakshz commented Aug 21, 2022

@LakshyaSatpal hey, you need a hand? I was out on holidays, but now back 💪🏼

Hi @derberg, still stuck now the same problem - not able to write a test with help flag 😥, discussed it with @Souvikns as well

@derberg derberg changed the title fix: improved help message of 'new' command fix: improve help message for new command Nov 21, 2022
studio: Flags.boolean({ char: 's', description: 'open in Studio' }),
port: Flags.integer({ char: 'p', description: 'port in which to start Studio' }),
'no-tty': Flags.boolean({ description: 'do not use an interactive terminal' }),
};

static args = [];

static examples = [
'asyncapi new\t - start creation of a file in interactive mode',
'asyncapi new --file-name=my-asyncapi.yml --example=default-example.yml --no-tty\t - create new file with specific name, using one of examples and without interactive emode'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'asyncapi new --file-name=my-asyncapi.yml --example=default-example.yml --no-tty\t - create new file with specific name, using one of examples and without interactive emode'
'asyncapi new --file-name=my-asyncapi.yml --example=default-example.yml --no-tty\t - create a new file with a specific name, using one of the examples and without interactive mode'


test
.stdout()
.command(['new', '-h'])
Copy link
Member

Choose a reason for hiding this comment

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

ok, we need to go forward here:
@LakshyaSatpal please look at other comments, address them, and remove the test. There is no way to make it run now. The main functionality will have to stay untested 😄

also please add "additionalHelpFlags": ["-h"], to oclif configuration in package.json (line 97) so we at least make -h to work too.

@derberg
Copy link
Member

derberg commented Dec 8, 2022

@LakshyaSatpal I hope you are still interested with this one. It helps with DX and as you see it is ok that we drop testing part

@lakshz lakshz requested a review from derberg December 8, 2022 14:01
@derberg derberg changed the title fix: improve help message for new command feat: improve help message for new command Dec 12, 2022
derberg
derberg previously approved these changes Dec 12, 2022
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

looks great from my point of view. Tried locally and works like a charm

(improve-new-help-msg)$ bin/run new -h
Creates a new asyncapi file

USAGE
  $ asyncapi new [-h] [-n <value>] [-e <value>] [-s] [-p <value>] [--no-tty]

FLAGS
  -e, --example=<value>
      name of the example to use. Available examples are:
        - simple.yml
        - anyof.yml
        - application-headers.yml
        - correlation-id.yml
        - websocket-gemini.yml
        - gitter-streaming.yml
        - mercure.yml
        - not.yml
        - operation-security.yml
        - oneof.yml
        - rpc-client.yml
        - rpc-server.yml
        - slack-rtm.yml
        - tutorial.yml
        - streetlights-kafka.yml
        - streetlights-operation-security.yml
        - streetlights-mqtt.yml

  -h, --help
      Show CLI help.

  -n, --file-name=<value>
      name of the file

  -p, --port=<value>
      port in which to start Studio

  -s, --studio
      open in Studio

  --no-tty
      do not use an interactive terminal

DESCRIPTION
  Creates a new asyncapi file

EXAMPLES
  $ asyncapi new         - start creation of a file in interactive mode

  $ asyncapi new --file-name=my-asyncapi.yml --example=default-example.yml --no-tty      - create a new file with a specific name, using one of the examples and without interactive mode

@Souvikns wanna have a look?

@derberg
Copy link
Member

derberg commented Dec 15, 2022

@Souvikns ping 😉

Souvikns
Souvikns previously approved these changes Dec 15, 2022
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

LGTM! just need to fix lint

@derberg
Copy link
Member

derberg commented Dec 20, 2022

@LakshyaSatpal hay can you fix linter issues please

@lakshz lakshz dismissed stale reviews from Souvikns and derberg via 022cfbf December 21, 2022 20:12
@sonarcloud
Copy link

sonarcloud bot commented Dec 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lakshz lakshz requested a review from derberg December 21, 2022 20:13
@derberg derberg closed this Feb 6, 2023
@derberg derberg reopened this Feb 6, 2023
@sonarcloud
Copy link

sonarcloud bot commented Feb 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member

derberg commented Feb 6, 2023

I have no idea why tests are failing 😭
it is not random, it is always [ERR_INVALID_ARG_TYPE]: The "warning" argument must be of type string or an instance of Error. Received an instance of TypeError but why.......#$%

@LakshyaSatpal maybe you could switch to readFile that is already used in the same file, instead of importing readFileSync? or was sync selected because description: await getExamplesFlagDescription() was not possible?

@derberg
Copy link
Member

derberg commented Mar 27, 2023

@LakshyaSatpal not sure why but you are having a lot of linter issues. Do you want to continue with this PR?

@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member

derberg commented Apr 24, 2023

@LakshyaSatpal can you specify if you plan to continue with this one, or should that be released to other contributors?

@derberg
Copy link
Member

derberg commented Jul 19, 2023

no reaction so closing, hope someone will pick it up from #162

@derberg derberg closed this Jul 19, 2023
@derberg
Copy link
Member

derberg commented Sep 4, 2023

done with #758

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.

Improve help message with list of available examples
3 participants