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

refactor: update API according to latest changes #54

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Mar 24, 2022

Description
Part of #53

This PR updates the current API with the latest changes applied in https://github.com/asyncapi/parser-js/tree/next-major and in asyncapi/parser-js#496.

I hope did not miss any other change 😅 .

cc @magicmatatjahu @Souvikns

Related issue(s)
#53

Souvikns
Souvikns previously approved these changes Mar 24, 2022
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

This solves a lot of my doubts about the server object ❤️

@smoya
Copy link
Member Author

smoya commented Mar 24, 2022

This solves a lot of my doubts about the server object ❤️

Thanks! 🎉 I decided to move into this task because of that reason in particular. Anyway, this API can contain errors so we should be able to detect them as soon as possible, that's why we are prioritizing implementation time over design time.

docs/v1.md Show resolved Hide resolved
@smoya
Copy link
Member Author

smoya commented Mar 28, 2022

@magicmatatjahu @jonaslagoni anything you see potentially wrong?

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Think there are some leftovers 🙂 Might be intentional 🤔?

- servers() : `Server[]`
- summary() : `string` | `undefined`
- tags() : `Tag[]`
- tags() : `Tags`
Copy link
Member

Choose a reason for hiding this comment

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

Make sure you are consistent either use a wrapper object or a simple array, don't use both IMO. I.e. we use channel[]

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Good catch.

docs/v1.md Outdated
## Message
- binding(bindingProtocol) : `any` | `undefined`
- bindingProtocol : `enum{'amqp', 'amqps', 'http', 'https', 'jms', 'kafka', 'kafka-secure', 'mqtt', 'secure-mqtt', 'stomp', 'stomps', 'ws', 'wss'}`
- bindings() : `Bindings`
- channels() : `Channel[]`
Copy link
Member

Choose a reason for hiding this comment

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

Is this left over?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Good catch.

docs/v1.md Outdated
- hasTitle() : `boolean`
- headers() : `Schema` | `undefined`
- id() : `string`
- name() : `string` | `undefined`
- operations() : `Operation[]`
Copy link
Member

Choose a reason for hiding this comment

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

Same here? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Good catch.

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@smoya
Copy link
Member Author

smoya commented Mar 28, 2022

Should Schema functions return Schemas object or rather an array of Schema ?

I.e. allOf : ˜Schema[]˜? I think Schema[] simplifies a lot, however because of consistency maybe we should switch to use the intermediate object Schemas.

What do you think @magicmatatjahu @jonaslagoni @Souvikns ?

@jonaslagoni
Copy link
Member

jonaslagoni commented Mar 28, 2022

I would not focus on creating the intents for JSON Schema, and keep it Schema.

@smoya
Copy link
Member Author

smoya commented Mar 28, 2022

I would not focus on creating the intents for JSON Schema, and keep it Schema.

We can always change it before releasing the final API anyway so 👍 for it

@smoya
Copy link
Member Author

smoya commented Mar 28, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 1bd653f into asyncapi:master Mar 28, 2022
@smoya smoya deleted the update branch March 28, 2022 13:32
@magicmatatjahu
Copy link
Member

I see I didn't comment in time 😄 But I agree with Jonas, Schema Object should work in a recursive way.

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants