Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor: add models for server object #497
refactor: add models for server object #497
Changes from 12 commits
763da46
6a98d2a
2099076
ecaa181
6523013
20dea91
d4ed90b
68b0982
2330aba
798db97
101cad0
a9fa78c
5033ba1
2371265
95c130a
356d7b0
d17d219
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had discussion in Slack that
id
should not be a part of_json
:) I also gave you example how it should work - https://github.com/asyncapi/parser-js/blob/next-major/src/models/v2/mixins/extensions.ts#L11There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because I modeled the utils function to convert object to array something like this
so I thought maybe I don't need the add a separate id variable can just get it to format it. Main reason was so that the
v2
andv3
have minimal changes, because to add seperate_id
parameter now we have to pass something likenew Servers(id, this._json)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v3 changes may be much bigger than they are now (to be honest we don't have anything approved so v3=v2 now), so please don't look at this proposal from Fran as something final.
About
id
. Have in mind that if someone will runjson()
should get the value that is defined for the object in the specification (not including traits because we have to combine them) without any additional fields that are not defined in the object, so what you want to get with adding theid
field is nothing more than changing the original json. I know that adding an id as an argument to a constructor can be weird, but in this case the id is outside the object, and in v3 it can be part of the object and it will be easier to handle.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smoya what is the
name
method in the server - https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#server?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
name
is the key in the map of servers. But I think we can change it to ID to be consistent?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is
id
then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so
name
andhasName
is out @SouviknsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then what is
id
? I was thinking thatid
is the keyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to use
id()
for all objects like Channel, Server, Operation, etc, instead ofpath()
for Channels,name
for Servers, etcIf we add a ID method for all of those models, no matter if we later change the source of them between versions.
I know the parser-api doesn’t reflect that but we learnt a lot during the last few months :)