-
-
Notifications
You must be signed in to change notification settings - Fork 211
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: map schema references from baseUrl to folder #529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it! I'd only suggest removing all these console.log
before it's merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good! Please refer to my comments and also, please write some integration tests using examples from your PR in test/docs
:)
@muenchhausen Answering to your questions:
You can write test only for
If you move new logic from constructor to separate function, it should be fixed. |
Please add information about new CLI param to the readme |
@muenchhausen please have a look at the failing tests |
I did not manage to create an integration test that checks the renedered result for Here is the test I tried: const path = require('path');
const Generator = require('../lib/generator');
//jest.mock('../lib/filtersRegistry');
//jest.mock('../lib/hooksRegistry');
describe('Generator', () => {
describe('#generateFromFile', () => {
it('test', async () => {
const gen = new Generator('@asyncapi/html-template', '/Users/z0024p1y/prototypes/generator/build', {
mapBaseUrlToFolder: 'https://schema.example.com/crm/:./test/docs/',
forceWrite: true
});
await gen.generateFromFile(path.resolve('./test/docs/apiwithref.json'));
});
});
}); |
@muenchhausen Hi! Thanks for your work! I see that you have very old revision in tests in your branch:
Please update branch and write tests like here https://github.com/asyncapi/generator/blob/master/test/integration.test.js :) |
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
I implemented all suggestions and tested this branch successfully in a company CI. I think I am done again :) |
// it splits on last occurrence of : into the groups all, url and folder | ||
const re = /(.*):(.*)/g; | ||
let mapping = []; | ||
if ((mapping = re.exec(v))===null || mapping.length!==3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a comment that this statement will not work with such situation:
ag test/docs/apiwithref.json @asyncapi/html-template -o ./build/ --force-write --map-base-url https://schema.example.com/crm/
because https://schema.example.com/crm/
has :
so const re = /(.*):(.*)/g;
returns 3 elements and therefor it contunes to work and fails when trying to resolve refs
Something went wrong:
Error: Error downloading https://schema.example.com/crm/shared.json
getaddrinfo ENOTFOUND schema.example.com
at dereference (/Users/wookiee/sources/generator/node_modules/@asyncapi/parser/lib/parser.js:155:11)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - good point! I fixed it. Of course it would be better to put cli.js under test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we will have all that is needed in the new CLI that will be not only for the generator but also other tools -> https://github.com/asyncapi/cli
this is why we do not invest much in CLI tests here
@muenchhausen I played with this on local and just found this https://github.com/asyncapi/generator/pull/529/files#r629460080. Other than that - great stuff 🎉 @fmvilas @magicmatatjahu have a final look folks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🚀
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@all-contributors please add @muenchhausen for code |
I've put up a pull request to add @muenchhausen! 🎉 |
Description
introduce a new cli parameter
--map-base-url <url>:<baseFolder>
: maps all schema references with this base url to folderThis can be tested by running
Related issue(s)
Resolves #512
Open
cli.js
orgenerator.js
?