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: integrate cupid in cli using the relation command #143

Closed

Conversation

arjungarg07
Copy link

Description

This PR integrates @asyncapi/cupid in cli using the relation command.

Command usage syntax:
asyncapi relation <context-1/filepath1> <context-2/filepath2> <context-3/filepath3> ...

It has one flag:
--type which determines the output syntax of the relations and if not provided, would output default output syntax.

Related issue(s)
#47

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.

@fmvilas
Copy link
Member

fmvilas commented Dec 2, 2021

Great stuff, @arjungarg07! My only concern is that the command is not a verb. We decided to follow the form asyncapi VERB [NOUN] and relation is not a verb. What about visualize?

asyncapi visualize context1/asyncapi1 context2/asyncapi2 ...

@arjungarg07
Copy link
Author

arjungarg07 commented Dec 3, 2021

Yeah @fmvilas , I thought of using the verb visualize but I was skeptical about it since we are not actually visualizing the architecture right away but just getting the relations in desired output format and later pasting it in live editor to visualize.

@fmvilas
Copy link
Member

fmvilas commented Dec 7, 2021

🤔 I see what you mean. Maybe it should be something like asyncapi generate diagram? It kinda collapses with asyncapi generate where we probably want to use Generator. But maybe we can use asyncapi generate code and asyncapi generate docs for those...

For sure it can't be relation as it's not a verb/command, just a noun. Any other ideas?

@arjungarg07
Copy link
Author

What about asyncapi generate relations ?

@fmvilas
Copy link
Member

fmvilas commented Dec 8, 2021

🤔 It might sound as you're generating the relations but the relations are already there, you're just "finding" them and generating a diagram, isn't it?

@arjungarg07
Copy link
Author

I didn't think in that way, in that case asyncapi generate diagram would be good.

@fmvilas
Copy link
Member

fmvilas commented Dec 10, 2021

Yeah, now what worries me is that clash with asyncapi generate command. There's a related conversation going on in #129.

@sonarqubecloud
Copy link

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 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@felixjung
Copy link

felixjung commented Jan 18, 2022

What about asyncapi relate?

relate

to find or show the connection between two or more things

@arjungarg07
Copy link
Author

What about asyncapi relate?

Yeah, this could be done.
@derberg what do you think about it?

@derberg
Copy link
Member

derberg commented Jan 27, 2022

@arjungarg07 not sure if this is intuitive. Look at #129 (comment). I think it should go under generate because in the end you are generating something. It is about the outcome and not what the library is doing. You tell CLI to generate a diagram, and not find relations. At least this is how I look at it 🤔

@fmvilas have a look as you were involved here

@fmvilas
Copy link
Member

fmvilas commented Jan 27, 2022

Yeah, I still think it's better if we stick to generate diagram or similar. Always look at it from the point of view of the user who doesn't know the underlying technology and instead they expect the command to represent what they want to achieve. The tool used for this is irrelevant for them.

@Souvikns Souvikns mentioned this pull request Mar 3, 2022
16 tasks
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 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 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
Copy link
Member

derberg commented May 24, 2022

@arjungarg07 hey is this one ready for review?

  • can you remove 2 code smells?
  • can you remove manual bump of the cli version from package.json and package-lock.json, as we do it by the bot, and it will constantly cause issues for you on a PR, conflicts you do not want to have

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Sep 22, 2022
@derberg
Copy link
Member

derberg commented Nov 17, 2022

@arjungarg07 hey, not sure if you noticed but asyncapi generate is now possible, so you can continue with this PR if you want

@github-actions github-actions bot removed the stale label Nov 18, 2022
@arjungarg07
Copy link
Author

@derberg what would be the final command look like? asyncapi generate relation ?

Copy link
Member

derberg commented Nov 28, 2022

I think mostly suggested was diagram not relation

this could maybe also be integrated with EDAVisualizer @jonaslagoni ?

so one command, but underneath different tools would be used

@derberg
Copy link
Member

derberg commented Mar 27, 2023

@arjungarg07 yo, should we keep it open?

@arjungarg07
Copy link
Author

@derberg I had some doubts, to be resolved(asked above in the thread). Maybe, would open a new PR.

Copy link
Member

derberg commented Mar 28, 2023

up to you 😃

@derberg
Copy link
Member

derberg commented Apr 3, 2023

to be resolved(asked above in the thread)

oh wait, sorry, which doubts are not yet solved?

@derberg derberg mentioned this pull request Apr 3, 2023
@derberg
Copy link
Member

derberg commented Jul 19, 2023

@arjungarg07 I'm guessing we should close it, right?

@chinma-yyy
Copy link
Contributor

@arjungarg07 I'm guessing we should close it, right?

So does it mean that this issue is open to other contributors @derberg ??

Copy link
Member

derberg commented Jul 31, 2023

yes, but since Arjun is the maintainer of cupid, let's first confirm if this issue makes sense.

@arjungarg07 what are your plans with cupid, do you plan to continue maintain it?

Copy link
Collaborator

@Shurtu-gal Shurtu-gal left a comment

Choose a reason for hiding this comment

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

Also, the example folder can be moved into https://github.com/asyncapi/cli/tree/master/test/fixtures

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arjungarg07 there has been a recent change in the folder structure for tests in #712. This can be moved into https://github.com/asyncapi/cli/tree/master/test/integration.

@derberg
Copy link
Member

derberg commented Sep 12, 2023

closing for now, I'm in touch with @arjungarg07 to start a process of looking for new maintainers for cupid

@derberg derberg closed this Sep 12, 2023
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.

6 participants