Skip to content

feat: mock intent implementation#270

Closed
jonaslagoni wants to merge 72 commits into
asyncapi:masterfrom
jonaslagoni:feature/mock_implementation
Closed

feat: mock intent implementation#270
jonaslagoni wants to merge 72 commits into
asyncapi:masterfrom
jonaslagoni:feature/mock_implementation

Conversation

@jonaslagoni

Copy link
Copy Markdown
Member

Description

  • ...
  • ...
  • ...

Related issue(s)

Comment thread lib/parser.js
@smoya smoya force-pushed the feature/mock_implementation branch from 46cb147 to 2fe851e Compare April 6, 2021 07:46
Comment thread lib/intent-models/server-security.js Outdated

@jonaslagoni jonaslagoni left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@smoya and I have added all intents as we see they should be and tweaked a few things. However, we need your help in finalizing it.

I have included a review with questions I had while going over the API, feel free to add your own review as well, we want to hear your opinion 🙏

We suggest that we slightly change how we view the parser API or at least suggest how it should be seen. Instead of trying to make the API fit perfectly to the spec (so you can access all properties) what if we just focus on what makes sense from the event-driven point of view? We can still give access to the underlying model in the future, so tool developers always have the choice to go around the intent API in edge cases where this is the only choice. For example: why would we want to give access to traits? They are something for the spec to allow the inheritance of traits between messages etc.

Adopting this view these are the properties from the spec which are purposefully not added to the API:

  • Traits
    • Since whether a property comes from a trait or not should not matter from tooling perspective.
  • Message schema format
    • It is a property for parsers to know how to parse schemas, but is it relevant to include it in the API?
  • Components
    • What is the intent to only access messages, etc from components. I know some define messages long before channels are defined. But shouldn't those be accessed with AsyncAPI.messages()?

Questions:

  • Should we include isEmpty() support functions for those that return an array? Or should we still use hasX()? Related parser issue: #201
  • What about external docs. Would it not make sense to wrap all descriptions in a new model called Description which then contains all these different aspects of a description?
  • What functions should always return a value (i.e. not being optional)? Should we follow all required properties from the spec or? We need to remember that changing a value from required to optional is a breaking change, so the fewer the better.

Comment thread lib/intent-models/asyncapi.js Outdated
Comment thread lib/intent-models/asyncapi.js Outdated
Comment thread lib/intent-models/asyncapi.js Outdated
Comment thread lib/intent-models/info.js
Comment thread lib/intent-models/license.js
Comment thread lib/intent-models/message.js
Comment thread lib/intent-models/asyncapi.js
Comment thread lib/intent-models/channel.js
Comment thread lib/intent-models/channel.js
Comment thread lib/intent-models/server.js
Comment thread lib/intent-models/channel.js Outdated
@sonarqubecloud

Copy link
Copy Markdown

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions Bot added the stale label Jun 23, 2021
@github-actions github-actions Bot closed this Aug 22, 2021
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.

2 participants