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: issue with not reading spec file in working directory when no global context is created #48

Merged
merged 5 commits into from
Aug 17, 2021
Merged

Conversation

Souvikns
Copy link
Member

…obal context is set.

Description

  • Fixed issue with not reading spec file in the working directory when global context file is not created.

Related issue(s)

Fixes #35

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.

Works like a charm mate, but we need to put more power into developer experience

src/messages.tsx Show resolved Hide resolved
src/messages.tsx Outdated
@@ -14,3 +14,5 @@ export const ValidationMessage = (filePath: string) => ({
error: () => `File: ${filePath} does not exists or is not a file!`,
message: () => `File: ${filePath} successfully validated!`
});

export const NO_SPEC_PATH_FOUND = 'No spec path found in your working directory, please use flags or store a context';
Copy link
Member

Choose a reason for hiding this comment

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

We need to do better here. User, when running asyncapi validate most probably do not have an idea about all our options, defaults, and way we load files, priorities - we need to make it clear. Good help messages are a key for good developer experience.

how about? 👇🏼

Suggested change
export const NO_SPEC_PATH_FOUND = 'No spec path found in your working directory, please use flags or store a context';
export const NO_SPEC_FOUND = 'Unable to perform validation. Specify what AsyncAPI file should be validated.\n\nThese are your options to specify in the CLI what AsyncAPI file should be used:\n- You can use --file flag to provide a path to the AsyncAPI file: asyncapi validate --file path/to/file/asyncapi.yml\n- You can use --context flag to provide a name of the context that points to your AsyncAPI file: asyncapi validate --context mycontext\n- In case you did not specify a context that you want to use, the CLI checks if there is a default context and uses it. To set default context run: asyncapi context use mycontext\n- In case you did not provide any reference to AsyncAPI file and there is no default context, the CLI detects if in your current working directory you have files like asyncapi.json, asyncapi.yaml, asyncapi.yml. Just rename your file accordingly.';

Now, above is just a suggestion of how message should be printed, but from code perspective it should be splitted

Part Unable to perform validation. Specify what AsyncAPI file should be validated. should be only in context of asyncapi validation and the rest of the message should be reusable. Why reusable? because it will be always used in other commands when the same error will happen that there is no file specified, when we will have command asyncapi optimize it will be the same. So you need to make sure that examples that I provided in the message are customizable, in context of validation user see asyncapi validate --file path/to/file/asyncapi.yml, but in context of optimization user will see asyncapi optimize --file path/to/file/asyncapi.yml

@derberg
Copy link
Member

derberg commented Aug 12, 2021

@Souvikns please have a look at my comments again, I suggested that part of the message must be reusable and things like asyncapi validate cannot be hardcoded

@Souvikns
Copy link
Member Author

@derberg I took an approach to make the message reusable, let me know if this works or if we should take a different approach.

Looking at the message I would see only the name of the command in the example is dynamic. Suppose I am using the validate command and this error is thrown then the message will have asyncapi validate --file path/to/file/asyncapi.yml, on the contrary if I am using the optimize command then asyncapi optimize --file path/to/file/asyncapi.yml will be printed.

So basically we could pass a command parameter to the NoSpecPathFoundError to be used in the NO_SPEC_FOUND message. But then we needed to somehow access the command in run time. For that, I created a class to access the command name being used.

import meow from 'meow';
import { injectable } from 'tsyringe';

injectable();
export class CLIService {
  private _cli = meow('', { argv: process.argv.slice(2), autoHelp: false })

  command(): string {
    return this._cli.input[0] || '';
  }

  args(): any {
    return this._cli.flags;
  }
}

@derberg
Copy link
Member

derberg commented Aug 16, 2021

NO_SPEC_FOUND has this part "hardcoded" to validation -> Unable to perform validation. Specify what AsyncAPI file should be validated

I won't be able to reuse it with optimize or other new commands, or? I think this part should be separated

@Souvikns
Copy link
Member Author

maybe we can make an object which stores all these fallback messages and after we determine which command was used print them accordingly.

@derberg
Copy link
Member

derberg commented Aug 16, 2021

Can't we just split NO_SPEC_FOUND into NO_SPEC_FOUND and VALIDATION_NO_SPEC_FOUND and in context of validation, concat both and in case of optimization have OPTIMIZE_NO_SPEC_FOUND? later once more come and it gets cumbersome we refactor

@Souvikns
Copy link
Member Author

If we divide the complete message then Unable to perform validation. Specify what AsyncAPI file should be validated. part will be different for every command.

- You can use --file flag to provide a path to the AsyncAPI file: asyncapi validate --file path/to/file/asyncapi.yml
- You can use --context flag to provide a name of the context that points to your AsyncAPI file: asyncapi validate --context mycontext
- In case you did not specify a context that you want to use, the CLI checks if there is a default context and uses it. To set default context run: asyncapi context 
use mycontext
- In case you did not provide any reference to AsyncAPI file and there is no default context, the CLI detects if in your current working directory you have files    
like asyncapi.json, asyncapi.yaml, asyncapi.yml. Just rename your file accordingly.

We can split them and then connect them in the error to use accordingly.

@derberg
Copy link
Member

derberg commented Aug 16, 2021

exactly, split and then reuse together or in different configurations

@Souvikns Souvikns requested a review from derberg August 16, 2021 10:38
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.

Great work!

@derberg derberg merged commit b7b831e into asyncapi:master Aug 17, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Souvikns Souvikns deleted the cwd-error-patch branch September 9, 2021 20:20
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.

Validation of default files in CWD does not work when I have no files in the context
3 participants