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

feat: created specificationUrl class #110

Merged
merged 17 commits into from
Jan 4, 2022
Merged

feat: created specificationUrl class #110

merged 17 commits into from
Jan 4, 2022

Conversation

Souvikns
Copy link
Member

@Souvikns Souvikns commented Nov 2, 2021

Description
This PR aims to add support to fetch specification file from url.

  • Ultimately the cli will be able to take url as a parameter to resovle commands
  • Save url in context
  • Fetch specification file from url.

Related issue(s)

Resolves #7

@Souvikns
Copy link
Member Author

Souvikns commented Nov 2, 2021

load function is returning SpecificationFile instance but to add support for URL paths it feels weird to have fetch function in the SpecificationFile. We could create another SpecificationUrl class that will fetch the data, but I think we should change SpecificationFile to Specification and have two separate functions loadFromFile and loadFromUrl to load the spec file accordingly.

So our load function would look something like this

const spec = await load(filePath);
await parser.parser(spec.text());

need your opinion on this @fmvilas

src/models/SpecificationUrl.ts Outdated Show resolved Hide resolved
src/models/SpecificationFile.ts Show resolved Hide resolved
@fmvilas
Copy link
Member

fmvilas commented Nov 3, 2021

In general, my advice is that we should be reducing the cognitive load of the code, and I think creating more classes and more modules that depend on each other only makes it harder to understand. Remember what we talked about the "hops" you have to do when reading a code. If the only logic that's really different is validating a URL and fetching from a URL, then probably all you need are just "helpers" or external modules that already do that.

@sonarcloud
Copy link

sonarcloud bot commented Nov 3, 2021

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

@Souvikns Souvikns marked this pull request as ready for review December 13, 2021 05:09
@Souvikns
Copy link
Member Author

@fmvilas should startStudio function take file path as input or spec as input. To create URL support I was thinking of making it in a way that every command which needs to read specification from somewhere can just use the load function but it will not get file path or URL or context but rather the specification string or json itself. Studio needs filepath and port to start so it does not support URL as of now.

Also Should studio load specification from URL as it won't be able to update it anyway.

@fmvilas
Copy link
Member

fmvilas commented Dec 13, 2021

Yeah, I don't think Studio needs to be able to open a URL, at least for now. As you said, you'll not be able to edit it anyway so what's the point 🤷‍♂️

test
.stderr()
.stdout()
.command(['validate', 'https://bit.ly/asyncapi'])
Copy link
Member

Choose a reason for hiding this comment

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

I see that @fmvilas already reviews, so from my point of view I only suggest to log a followup issue, something good for the first contribution, to make sure tests do not rely on external resources. Basically, external resources are risky to use in tests and can cause tests to be flaky, like randomly failing just because the external resource is not available. So follow up should describe that tests should actually start independent server hosting asyncapi file, like we do in parser -> https://github.com/asyncapi/parser-js/blob/master/package.json#L20

@derberg
Copy link
Member

derberg commented Dec 14, 2021

From my point of view important is that help message spec path or context-name is updated

src/commands/validate.ts Outdated Show resolved Hide resolved
src/commands/validate.ts Outdated Show resolved Hide resolved
src/models/SpecificationFile.ts Outdated Show resolved Hide resolved
return this.filePath;
}

getURLPath() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you shouldn't call this "URL path". That's a URL.

I mean, this is a URL: https://www.asynapi.com/blog.
And this is the URL path: /blog.

They're not the same and I think here you mean the full URL, not just the path part.

Why not getFileURL()?

Suggested change
getURLPath() {
getFileURL() {

If you change it, you'd have to change everything else following similar notation, like const TYPE_URL_PATH = 'url-path';

derberg
derberg previously approved these changes Dec 21, 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.

All good from my side but wait for @fmvilas as he made some good quality review

also you have some merge conflicts in package.json

@derberg
Copy link
Member

derberg commented Dec 27, 2021

@fmvilas as you initially requested changes you need to approve before we can merge

@sonarcloud
Copy link

sonarcloud bot commented Dec 27, 2021

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

@magicmatatjahu
Copy link
Member

Looking at the changes, it's ok for me. You just have to wait for the Fran :)

@fmvilas
Copy link
Member

fmvilas commented Jan 3, 2022

Sorry for the delay. Looks good to me 🚀

@Souvikns Souvikns merged commit 0dd0cb8 into asyncapi:master Jan 4, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.12.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.

Support passing url to AsyncAPI file
5 participants