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

fix: dereference is failing with url references #69

Merged
merged 5 commits into from
Apr 20, 2020

Conversation

fmvilas
Copy link
Member

@fmvilas fmvilas commented Apr 17, 2020

This is a temporary fix meanwhile the folks at @APIDevTools merge this pull request or another one solving the same problem.

The problem is that when a $ref like https://myserver.com/mySchema.json fails to download (either because it doesn't exist or another kind of errors), the information we get about the error is not that much. We get a message, a status, and some other information but we don't know which $ref provoked this error, i.e., the https://myserver.com/mySchema.json.

We need to know which $ref provoked the error so that we're able to locate it in the document and give a nice hint to the user on how and where the error is.

Update

I closed my PR because it doesn't make sense anymore. However, I can't still stop pointing to my fork because of this: APIDevTools/json-schema-ref-parser#164.

package.json Outdated
@@ -43,11 +43,11 @@
"semantic-release": "^17.0.4"
},
"dependencies": {
"@apidevtools/json-schema-ref-parser": "github:fmvilas/json-schema-ref-parser",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is pointing to my fork until the other PR gets merged.

@fmvilas fmvilas requested review from derberg and jonaslagoni April 17, 2020 17:53
derberg
derberg previously approved these changes Apr 19, 2020
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.

just few questions:

  • you take dependency from your fork but from master, why not add-more-info-about-errors that you used for the PR to apidevtools (btw you have conflicts there)
  • when I look on the yaml with example of ref broken by new line, I think it shoudl be added to tck. I'm not saying you should do it now. I'm thinking now that tck should actually become a library so you can have it in your devDependencies and inject into the tests, for example parser tests. What do you think?

I approve, well done,
I approve cause I assume you want to ref master of the fork in dependencies and you will care to always synchronize fork-branch with fork-master (although making sure your fork is up to date with upstream will be difficult)

@jonaslagoni
Copy link
Member

Already notified @fmvilas but the tests does not succeed in Windows. Just wanted to make sure it won't get merged untill fixed 😄

@fmvilas
Copy link
Member Author

fmvilas commented Apr 20, 2020

@derberg Good call. I pointed to the branch on the fork. And agree about the TCK. It would be awesome to have it running on Github Actions too.

@jonaslagoni Tests are now fixed and taking into account EOL's format (/r/n or /n).


I also closed my PR because the Stoplight folks added a new feature to this library and, on the way, solved the problem I was fixing (perfect!). However, I'm still pointing to my fork because there's a bug on their implementation: APIDevTools/json-schema-ref-parser#164. Therefore, I'll keep using my fork until it gets fixed and released.

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.

lgtm

@fmvilas fmvilas merged commit a34028c into asyncapi:master Apr 20, 2020
@fmvilas fmvilas deleted the fix-errors-on-ref-resolving branch April 20, 2020 14:10
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.18.1 🎉

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.

4 participants