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: add new context command to make it easier to work with multiple files #14

Merged
merged 34 commits into from
Jul 9, 2021
Merged

Conversation

Souvikns
Copy link
Member

Description

  • Create commands to interact with the CLI for context operations
  • Successfully create and maintain a .asyncapi file at the home directory to persist the context.
  • Create functions or layers to support other commands to use the saved context for their operation.

Related issue(s)

Fixes #8

@Souvikns Souvikns changed the title writing logic for persisting context. feat: writing logic for persisting context. Jun 18, 2021
@Souvikns Souvikns marked this pull request as ready for review June 22, 2021 08:58
@Souvikns
Copy link
Member Author

@derberg it's not complete, can you check if it is going in the right direction.

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.

looking awesome, so clean! just left some comments.

@jotamusik man, you did great job with initial setup

src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/hooks/context/contextService.spec.ts Outdated Show resolved Hide resolved
@Souvikns
Copy link
Member Author

I think we should persist in the context not globally in the home directory but rather project level.

Scenario

Suppose I have multiple separate projects in my system, and I have installed the CLI as a dev dependency on all my projects. So I am using individual CLI instances for my different projects. They all might have different versions. Saving the context .asyncapi file in the home directory, now all the CLI instances need to access that one file. I think the user will need to save the update the file frequently if changing projects often. But if we save the .asyncapi file in the project level that is in the node_module/@asyncapi/CLI/.asyncapi then the users don't have to update even if changes project.

what do you think @derberg

@derberg
Copy link
Member

derberg commented Jun 24, 2021

@Souvikns this must be validated with tools like brew or chocolatey. I don't know if node_module/@asyncapi/CLI/.asyncapi could be supported if you provide a distro for brew for example.

Thing is that CLI users are not JS only, and there are many non-JS devs that consider it a shame to install nodejs 🤷🏼 I'm not gonna judge 😄 we need to respect that and to get wider CLI adoption, try making in available without forcing people to use npm or npx or anything like that. Thus the home location instead of node_modules, because I assume later is not supported with my scenario.

src/CommandsRouter.spec.tsx Show resolved Hide resolved
src/components/Context/Context.tsx Outdated Show resolved Hide resolved
src/components/Context/current.tsx Outdated Show resolved Hide resolved
src/components/Context/list.tsx Outdated Show resolved Hide resolved
src/hooks/context/models.ts Outdated Show resolved Hide resolved
src/hooks/context/hook.tsx Outdated Show resolved Hide resolved
@Souvikns
Copy link
Member Author

Souvikns commented Jun 28, 2021

@jotamusik I tried something with the command router can you check once if it's fine. It is still a work in progress.

@Souvikns Souvikns requested a review from derberg June 28, 2021 08:40
@Souvikns
Copy link
Member Author

@derberg I have changed the architecture of the hook

now every function will return a response object and an error object accordingly if there won't be any error then the error object will be undefined and if there will be an error the response object will be undefined.

let {list, err} = useContexFilet().list();

let {specFile, err} = useContextFile().loadSpecFile();

let {contextFile, err} = useContextFile().addContext(key, value);

let {contextFile, err} = useContextFile().changeCurrent("home");

let {contextFile, err} = useContextFile().remove();

@derberg
Copy link
Member

derberg commented Jul 1, 2021

Hey, sorry for the late review but was super busy with 2.1 spec release.
I have some comments:

  • If I have no context set, I should get a clear info about it when doing asyncapi context current. Now there is no message, just silent execution. I should also get a suggestion how to set a context
  • When I do asyncapi context list and do not have context file, I should get a nice message that I have no context, user should not care if context are in some file or no, this is Context file not found in the home directory technical information that is not relevant, more relevant is that I have no context and how to add one
  • asyncapi context remove exacutes silently if I have no context. I should get a clear message + we should support removing of another context from the list, not just the current.
  • we have option set but in code we always call it add, rename to add is better imho
  • I don't think set works as expected:
    $ asyncapi context set test https://raw.githubusercontent.com/asyncapi/generator/master/test/docs/dummy.yml
    $ asyncapi context list
    Context file not found in the home directory
    $ asyncapi context current
    $ 
    

I believe you are missing ContextService and context related components are not really used

@Souvikns
Copy link
Member Author

Souvikns commented Jul 1, 2021

@derberg Context Releated component I am yet to hook up with the router was am waiting to hear from @jotamusik as he suggested changes for the router logic to be shifted from the context components itself.
In the meantime, I am fixing all other issues and creating better messages.

@derberg
Copy link
Member

derberg commented Jul 1, 2021

@Souvikns sound good, ping me when you need me

@Souvikns
Copy link
Member Author

Souvikns commented Jul 2, 2021

@derberg I have moved to ContextService, having doubt about the naming convention for the removal of the context and the current.

  • removing the current context
  • removing an existing context key-value pair.

@derberg
Copy link
Member

derberg commented Jul 2, 2021

@Souvikns I think I need more data here, what exactly you have a challenge with?

@Souvikns
Copy link
Member Author

Souvikns commented Jul 2, 2021

I just need some opinion from you about the command naming with deletion of a context and clearing the current

I was thinking something like this
context delete <key> to delete any context key-value pair
context clear to clear the current context

@derberg
Copy link
Member

derberg commented Jul 9, 2021

ok then, autodetection of files or context will be done in a separate PR. I see but fixes are solved. The URL fetching I would drop for now

@derberg derberg changed the title feat: writing logic for persisting context. feat: add logic for persisting context Jul 9, 2021
README.md Outdated Show resolved Hide resolved
src/CliModels.ts Outdated Show resolved Hide resolved
src/cli.ts Show resolved Hide resolved
src/constants.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.

  • manual tests 🚀
  • there are some issues that are not much related to this PR or can be done later, I wrote about followup issues, please create them 💪🏼
  • what we definitely need to fix before merge is
    • making sure we check file exists before we add it to the context
    • make sure tests do not messup with global context

so far so good!

src/components/Context/Context.tsx Outdated Show resolved Hide resolved
src/components/Context/Context.tsx Outdated Show resolved Hide resolved
src/components/Context/Context.tsx Show resolved Hide resolved
src/components/Context/context.spec.tsx Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/hooks/context/hooks.tsx Outdated Show resolved Hide resolved
@jotamusik
Copy link
Contributor

Also, be aware that the default tab size is 2 spaces and there are some new files that have a tab size of 4 instead. 😅

@derberg
Copy link
Member

derberg commented Jul 9, 2021

@jotamusik

Also, be aware that the default tab size is 2 spaces and there are some new files that have a tab size of 4 instead. 😅

you sure you wanna do a tab-fight on Friday 😆

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.

LGTM 🚀

@derberg derberg changed the title feat: add logic for persisting context feat: add new context command to make it easier to work with multiple files Jul 9, 2021
@derberg derberg merged commit b3f2b56 into asyncapi:master Jul 9, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@derberg
Copy link
Member

derberg commented Jul 9, 2021

@Souvikns please do not forget to create all the follow-up issues that were mentioned here in the comments and also on slack. Remember that first improvement that we need to do here is to make sure tests do not operate on global .asyncapi context. It should be possible to manipulate it and I think it will be super easy just to address it through environment variable. So basically .asyncapi at home dir is default but one can override it with env like CLI_HOME_PATH=./path, so I run it as CLI_HOME_PATH=./path asyncapi context current. This should do the trick

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.

Persisting context for multiple projects
4 participants