-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
Conversation
Breaking the help commands into different parts.Taking inspiration from
|
looks great, but remember, not in scope of this PR 😉
we need also a sentence that explains what context is
Rather
I don't think it should be external. when I run |
@derberg I have converted all the error messages into constants and created a |
There was a problem hiding this 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
There was a problem hiding this 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
@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? |
@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 |
I was thinking of creating a complete class for responses, just like I did for the help message text. |
For HelpMessage it makes sense as there is a lot of reusability, the other messages are pretty independent. Class would be an overhead imho |
@derberg can we run the tests again, all the test cases are running in my local setup. |
@Souvikns look at the error:
looks like test picked up some old instance that has |
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. |
There was a problem hiding this 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
ready for last review round? |
Two minor things left to update, then It will be ready. |
@derberg I am ready for the last review round. 🤞🏻 |
There was a problem hiding this 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!
There was a problem hiding this 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?
There was a problem hiding this 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
🎉 This PR is included in version 0.3.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Turning CLI messages into constant so that it would be easy to update in the future.
Related issue(s)
Resolves #21