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: cleanup interfaces + add missing methods from parser-api #597

Merged
merged 5 commits into from
Sep 12, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Sep 7, 2022

Description

This PR does a cleanup and sync with the latest changes on parser-api made in asyncapi/parser-api#71 (comment).

Related issue(s)

Comment on lines 5 to 6
export type SecuritySchemaType = 'userPassword' | 'apiKey' | 'X509' | 'symmetricEncryption' | 'asymmetricEncryption' | 'httpApiKey' | 'http' | 'oauth2' | 'openIdConnect' | 'plain' | 'scramSha256' | 'scramSha512' | 'gssapi';
export type SecuritySchemaLocation = 'user' | 'password' | 'query' | 'header' | 'header' | 'cookie';
Copy link
Member

Choose a reason for hiding this comment

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

Hmm 🤔 Probably we should move that types to the spec-types/v2. SecuritySchemaType probably I wrote but we have missed SecuritySchemaLocation here. Could you move them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are already in spec-types/v2. The question is, should we also add all of them (meaning all locations possible, no matters the version of the spec) in the interface?That was the idea behind this in parser-api: https://github.com/asyncapi/parser-api/pull/71/files#diff-ac35ac6e0b0ad3629802a0e04076aa18e94da614f5392e1164dcaacdfe3ee198R375.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we don't make different spec types for 2.0.0, 2.1.0 etc. v2 spec types is for 2.x.x, so please use types from spec-types/v2.

Copy link
Member Author

Choose a reason for hiding this comment

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

v2 spec types is for 2.x.x, so please use types from spec-types/v2

But we are talking about types located in the interface (src/models/security-scheme.ts). Not related with the implementation.

Copy link
Member Author

@smoya smoya Sep 12, 2022

Choose a reason for hiding this comment

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

Do you mean we should just use string as type here, and in the implementation (v2, v3...) to set those values as unique possible values for that string?

Copy link
Member

Choose a reason for hiding this comment

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

We can even use v2.SecuritySchemaLocation | v3.SecuritySchemaLocation type in the future, but probably better will be string and then in implementation cast type to the v2.SecuritySchemaLocation. Please change to the string and move types to the spec-types/v2

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

src/models/v2/extensions.ts Outdated Show resolved Hide resolved
src/models/v2/extensions.ts Outdated Show resolved Hide resolved
smoya and others added 4 commits September 8, 2022 10:02
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
@smoya smoya requested review from magicmatatjahu and removed request for fmvilas, derberg, jonaslagoni and asyncapi-bot-eve September 12, 2022 08:34
@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 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM!

@magicmatatjahu
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit a27ed4a into asyncapi:next-major Sep 12, 2022
@smoya smoya deleted the feat/cleanupInterfaces branch September 12, 2022 08:55
@magicmatatjahu magicmatatjahu mentioned this pull request Sep 12, 2022
20 tasks
magicmatatjahu pushed a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022
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.

3 participants