refactor: adjust mixins to the IntentAPI #499
Conversation
magicmatatjahu
requested review from
fmvilas,
jonaslagoni,
derberg and
asyncapi-bot-eve
as code owners
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Nice rework @magicmatatjahu 👏 💯 Just dropped a comment so we take a decision, but we can merge and act after anyway 👍
| @@ -0,0 +1,20 @@ | |||
| import type { BaseModel } from "./base"; | |||
|
|
|||
| export abstract class Collection<T extends BaseModel> extends Array<T> { | |||
| return doc && new License(doc); | ||
| } | ||
| // TODO: Implement it | ||
| id(): string | undefined { |
There was a problem hiding this comment.
I'm not sure if i like the fact we moved the id from the document to the info object In https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#info.
Making an abstraction exercise, would it make sense to instead leave it in the root document (as it is nowadays)? cc @magicmatatjahu @jonaslagoni
There was a problem hiding this comment.
The id specifies the unique id of a given AsyncAPI document, so for me the id is more of a document metadata for the user than a semantic value. Similarly with tags and external docs. They serve only as information for the user and I prefer this place than the root of the document - maybe we should move (in the 3.x.x) tags, id and externalDocs from root to the info?
There was a problem hiding this comment.
Interesting. So you are suggesting just going all through info object opposed to what I suggested. Also to maybe suggest such a change into the spec itself for v3 (not related to the work we are doing here, but interesting anyway).
I'm kinda sold, However, I would love to know what @jonaslagoni –who was also involved in the Intent-Driven API design process– thinks about.
There was a problem hiding this comment.
Think we talked about it as well, can't remember the reasons behind going with A or B. Think it had something to do with extensions, as we still needed a way to access those under asyncapi.info however, there can be other workarounds.
Fine by me, you will quickly realize if it makes sense or not once the API is in use 🙂
There was a problem hiding this comment.
but id etc. have nothing to do with extensions 😅 I guess i don't understand something.
There was a problem hiding this comment.
but id etc. have nothing to do with extensions 😅 I guess i don't understand something.
No, but the info grouping has. If you start to mix and which properties should be fetched under asyncapi.info.prop and others asyncapi.prop it serves as a bad experience, as you have a hard time guessing where you can access the information as a user. It won't be intuitive.
Therefore, I think we went with keeping everything under asyncapi.info.
But yea, I dont mind changing it, cause I like that approach as well, so 🤷
There was a problem hiding this comment.
but that's why we have the intent api (yeah? 😅 ), so that people don't have to know where the externalDocs are, whether in info or not. They only need to know that in version 2.x.x it was on the root, and in 3.x.x (maybe) in info, so from tolling perspective we won't have any braking changes.
Ok. I will merge that PR and we can continue discussion in this thread. Of course we can change API for id :)
There was a problem hiding this comment.
I'm up for moving everything into Info object as suggested by @magicmatatjahu. I'm thinking Info object could be pass around other functions in user's libraries for printing all the basic info of the application.
There was a problem hiding this comment.
@smoya We can create proposal with that for 3.0.0 :)
There was a problem hiding this comment.
Sure! feel free to open a new RFC with that. We will find champions for it!
|
/rtm |
Description
Related issue(s)
Part of #481
Part of #482