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: improve CLI messages and move them into separate file #40

Merged
merged 18 commits into from
Aug 11, 2021
Merged

fix: improve CLI messages and move them into separate file #40

merged 18 commits into from
Aug 11, 2021

Conversation

Souvikns
Copy link
Member

Description
Turning CLI messages into constant so that it would be easy to update in the future.

Related issue(s)
Resolves #21

@Souvikns Souvikns changed the title chore: Turning CLI messages to constant chore: turning CLI messages to constant Jul 27, 2021
@Souvikns
Copy link
Member Author

Souvikns commented Jul 27, 2021

Breaking the help commands into different parts.

Taking inspiration from commander.js we can divide every command to have top options and its subcommands.
so the top-level command would look something like this.

asyncapi --help

Usage: asyncapi [options] [command]

Options:
  -V, --version       output the version number
  -h, --help          display help for command

Commands:
  context [options]   Manage contexts
  validate [options]  validate spec file

asyncapi context --help

Usage: asyncapi context [options] [command]

Store key-value pair of contexts and the CLI will load your contexts 
automatically making your commands shorter. 

Options:
  -h, --help          display help for command

Commands:
  list                list all saved contexts
  current             see current context
  use <context-name>  set the context as current context
  add <context-name> <spec-file-path>   add/update new context
  remove <context-name>  remove a context

asyncapi validate --help

Usage: asyncapi validate [options]

validate spec file

Options:
  -f, --file <spec-file-path>         Path of the spec file
  -c, --context <saved-context-name>  context name to use
  -w, --watch                         watch mode
  -h, --help                          display help for command

Example:
 $ asyncapi validate --file=./asyncapi.yml
 $ asyncapi validate  // if context is set. 

@derberg does this looks good. Also I was thinking we should add an external link to the documentation of how CLI loads spec path automatically.

@derberg
Copy link
Member

derberg commented Jul 27, 2021

looks great, but remember, not in scope of this PR 😉

Store key-value pair of contexts and the CLI will load your contexts
automatically making your commands shorter.

we need also a sentence that explains what context is

validate spec file

Rather Validate an AsyncAPI file

Also I was thinking we should add an external link to the documentation of how CLI loads spec path automatically

I don't think it should be external. when I run asyncapi validate I don't want to see error that I have no contexts, I want to just get an info that I need to point to AsyncAPI file and a detailed explanation how I can do that, right there in the error, that I can use file flag with link to file, or context name with info how to get more help about context, or default context or default filename in CWD

@Souvikns
Copy link
Member Author

@derberg I have converted all the error messages into constants and created a Help class to generate help messages as needed.

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.

@Souvikns nice, looks super clean, just left 2 comments

src/messages.tsx Outdated Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
@derberg derberg changed the title chore: turning CLI messages to constant refactor: turning CLI messages to constant Jul 28, 2021
src/help-message.ts Outdated Show resolved Hide resolved
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.

what about messages from https://github.com/asyncapi/cli/blob/master/src/components/Context/contexterror.tsx ?

ultimate goal should be you never have to do things like this in tests: expect(lastFrame()).toMatch('No contexts saved yet.'); so basically never explicitly hardcode the message in the test but use a constant

@Souvikns
Copy link
Member Author

Souvikns commented Aug 2, 2021

@derberg I have converted the error messages into constants.

Some context commands have string responses like when a context is deleted. Shall I convert those into constant?

@derberg
Copy link
Member

derberg commented Aug 2, 2021

@Souvikns basically any message printed to the user must be a constant as it must be tested, thus it is used in code and tests, and should not be repeated. There are some tests for context and validation that are still using "hardcoded" messages, like Error Thrown, Error in validation, context deleted successfully or File: ${file.getSpecificationName()} successfully validated!

@Souvikns
Copy link
Member Author

Souvikns commented Aug 2, 2021

I was thinking of creating a complete class for responses, just like I did for the help message text.

@derberg
Copy link
Member

derberg commented Aug 2, 2021

For HelpMessage it makes sense as there is a lot of reusability, the other messages are pretty independent. Class would be an overhead imho

@Souvikns
Copy link
Member Author

Souvikns commented Aug 4, 2021

@derberg can we run the tests again, all the test cases are running in my local setup.
image

@derberg
Copy link
Member

derberg commented Aug 4, 2021

@Souvikns look at the error:

Expected substring: "No contexts saved yet, run asyncapi --help to know more"
    Received string:    "home : /Users/runner/work/cli/cli/test/specification.yml
    code : /Users/runner/work/cli/cli/test/specification.yml"

looks like test picked up some old instance that has .asyncapi file with some old test data. I rerun, but might be it won't work.

@Souvikns Souvikns requested a review from derberg August 4, 2021 17:20
@derberg
Copy link
Member

derberg commented Aug 5, 2021

@Souvikns
Copy link
Member Author

Souvikns commented Aug 5, 2021

Again the macos-tests are failing.

  test('return validation service errors when the validation has failed', async () => {
    ValidationServiceMock().makeReturn(ValidationResponse.createError({ detail: 'Error in validation' }));

    const useValidateResponse = await useValidate().validate(fileThatExistsValidationInput);
    expect(useValidateResponse.success).toBeFalsy();
    expect(useValidateResponse.errors[0]).toEqual('Error in validation');
  });

There are some test cases that are checking for error messages from the parser. Should we need to make these errors constants.

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.

since we have now all the messages in one place I made some suggestions to make them sound better

src/help-message.ts Outdated Show resolved Hide resolved
src/help-message.ts Outdated Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
src/messages.tsx Show resolved Hide resolved
src/messages.tsx Show resolved Hide resolved
src/messages.tsx Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Aug 10, 2021

ready for last review round?

@Souvikns
Copy link
Member Author

Two minor things left to update, then It will be ready.

@Souvikns
Copy link
Member Author

ready for last review round?

@derberg I am ready for the last review round. 🤞🏻

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.

one more message left

export class SpecFileNotFoundError extends Error {
  constructor() {
    super();
    this.message = 'specification file not found in that path.';
  }
}

should be No AsyncAPI file found in {PATH}

best reuse message from validation, File: ${filePath} does not exists or is not a file!

src/messages.tsx Outdated Show resolved Hide resolved
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.

left just 2 comments and additional question: meow should be updated and use new help message right?

@Souvikns Souvikns requested a review from derberg August 11, 2021 02:50
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.

We are ready to merge. Good stuff 🚀

I will only update PR title to fix as we need patch release as it is not just refactoring but also fix to many messages wording

@derberg derberg changed the title refactor: turning CLI messages to constant fix: turning CLI messages to constant Aug 11, 2021
@derberg derberg changed the title fix: turning CLI messages to constant fix: improve CLI messages and move them into separate file Aug 11, 2021
@derberg derberg merged commit 14e94d8 into asyncapi:master Aug 11, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Souvikns Souvikns deleted the better-cli-messages branch August 12, 2021 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn CLI messages to constants
4 participants