Skip to content

chore: add parser function and corresponding unit tests #7

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

Merged
merged 9 commits into from
Jul 27, 2021

Conversation

aayushmau5
Copy link
Member

Description
This PR implements the Parser function which will be used to parse two AsyncAPI documents.

CC @vinitshahdeo

@aayushmau5 aayushmau5 changed the title feat: Add parser function and corresponding unit tests feat: add parser function and corresponding unit tests Jun 12, 2021
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

please remember that the diff interface will also need to support "bypassing" the parser. When integrated with the CLI or the Studio in the future, the flow can be that you request a diff and the base document that you want to chance against another one is already parsed, so diff interface should also accept AsyncAPIDocument. Just for the record, so we do not forget 😅

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.

Some comments :) Great job! Please wait for other reviews.

@magicmatatjahu
Copy link
Member

Please also start using chore: not feat: or fix: prefixes for PR. Currently you don't have anything that you can show to the users of package like docs, Readme, exported functions etc, so please commits with first implementation with chore: and in the next weeks we will make the 0.2.0 release.

@aayushmau5 aayushmau5 changed the title feat: add parser function and corresponding unit tests chore: add parser function and corresponding unit tests Jun 14, 2021
Copy link
Collaborator

@vinitshahdeo vinitshahdeo left a comment

Choose a reason for hiding this comment

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

@aayushmau5 Added a few comments. Please have a look.

if (isUrl(path)) {
return parseFromUrl(path);
}
return parse(await readDocument(path));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aayushmau5 What will happen if there is some error thrown by readFile promise? I think we should catch that error. What're your views on this?

Copy link
Member Author

@aayushmau5 aayushmau5 Jun 23, 2021

Choose a reason for hiding this comment

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

We thought about handling errors, but for the time being we have put it on hold.
Because in the future, we will have our own custom "Error Object" like the @asyncapi/parser has. And currently the error being thrown by NodeJS is alright for our purpose.

@magicmatatjahu
Copy link
Member

@aayushmau5 Will you handle that PR in upcoming days? It has one month. Where we have blocker here?

@aayushmau5
Copy link
Member Author

@magicmatatjahu I think there are no blockers as of now. This PR is good to be merged.

@magicmatatjahu
Copy link
Member

@aayushmau5 You have one bug :)

image

Please wrap URL constructor in the try catch statement

@aayushmau5
Copy link
Member Author

aayushmau5 commented Jul 26, 2021

It's not that. The new URL is just there, without being used. That's why it's throwing that error. For now, I'm returning this URL object. So, it should be good to go.

Also, let me fix that testing real quck.

@aayushmau5
Copy link
Member Author

aayushmau5 commented Jul 26, 2021

The windows check is failing due to their \r\n end of line thing, right?

The logs don't say anything.

@magicmatatjahu
Copy link
Member

@aayushmau5 You can replace all white chars to the empty string '' and check in this way expected and output value :)

@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
0.0% 0.0% Duplication

@aayushmau5
Copy link
Member Author

Yup. That was a windows error. Fixed it for now.

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 magicmatatjahu merged commit c3c51ee into asyncapi:master Jul 27, 2021
@aayushmau5 aayushmau5 deleted the parsing branch August 14, 2021 14:04
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

6 participants