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.

Sign up for GitHub

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

Jump to bottom

fix: add missing anonymous schema id's #43

Merged
merged 21 commits into from
Apr 9, 2020
Merged

fix: add missing anonymous schema id's #43

merged 21 commits into from
Apr 9, 2020

Conversation

jonaslagoni
Copy link
Member

fixes #42

@jonaslagoni jonaslagoni marked this pull request as ready for review March 8, 2020 21:15
@jonaslagoni jonaslagoni requested review from fmvilas and derberg March 8, 2020 21:16
@jonaslagoni jonaslagoni mentioned this pull request Mar 8, 2020
Merged
@jonaslagoni
Copy link
Member Author

I actually have not checked how it handles circle references should I create a test for that 🤔 ?

@fmvilas fmvilas added this to the Make JS parser stable milestone Mar 13, 2020
@jonaslagoni jonaslagoni requested review from fmvilas and derberg March 19, 2020 16:43
@jonaslagoni
Copy link
Member Author

Sorry, didn't see conflicts, give me 2 seconds.

derberg and others added 8 commits March 19, 2020 18:18
@derberg @jonaslagoni
Create FUNDING.yml
d306adc
@derberg @jonaslagoni
Add GitHub Actions powered stale bot and new contrib welcome (asyncap…
58cd7f6 
…i#45)

* Add welcome message handling with GH action
* Add stale issues handing GH action
@derberg @jonaslagoni
Fix stale GH action to use proper latest version
bfb78ed
@dependabot @jonaslagoni
Bump acorn from 5.7.3 to 5.7.4 (asyncapi#51)
1198ccd 
Bumps [acorn](https://github.com/acornjs/acorn) from 5.7.3 to 5.7.4.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@5.7.3...5.7.4)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@fmvilas @jonaslagoni
Extract RAML and OpenAPI schema parsers to their own package (asyncap…
0120e02 
…i#44)

* Extract RAML parser to its own package

* Remove RAML tests

* Extract OpenAPI too

* Add RAML and OpenAPI links to README
@fmvilas @jonaslagoni
0.15.0
dc9b1e9
@jonaslagoni
test: changed the expected JSON for recursive schema ids
bfd48df
@jonaslagoni
Merge branch 'master' into feature/recursive_schema_ids
58ea694
@jonaslagoni
Copy link
Member Author

I absolutely hate rebasing 🗡

@derberg
Copy link
Member

derberg commented Mar 20, 2020

@jonaslagoni is it like ready ready for review or better to leave it for Monday 😄 I say you pushed some additional things after requesting review. Maybe you want to work more on it over the weekend 😆

@jonaslagoni
Copy link
Member Author

@derberg it is ready, I just saw in the changes here on GitHub it had some weird formatting but seems like I couldn't change it 🤔 let me know if it is crucial and I'll take another look :)

derberg
derberg requested changes Apr 1, 2020
View reviewed changes
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.

Well done. I added only few suggestions.
I'm missing some tests in asyncapi_test.js though. I would expect:

  • extend should return an array with all the schemas used in the document with "nested json schema" case we discussed under the issue. or maybe a separate test?
  • would be also great so see some tests that makes sure assignUidToParameterSchemas works as expected

Sorry again for such a late review 😭 I owe you 🍺

lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
jonaslagoni and others added 2 commits April 3, 2020 16:09
@jonaslagoni @derberg
Changed variable to constant
070d4b3 
Co-Authored-By: Lukasz Gornicki <[email protected]>
@jonaslagoni @derberg
Changed variable to let
bfbed9c 
Co-Authored-By: Lukasz Gornicki <[email protected]>
jonaslagoni and others added 4 commits April 3, 2020 16:12
@jonaslagoni @derberg
Changed variable to constant
0c44bab 
Co-Authored-By: Lukasz Gornicki <[email protected]>
@jonaslagoni @derberg
Update lib/models/asyncapi.js
80dd7d9 
Co-Authored-By: Lukasz Gornicki <[email protected]>
@jonaslagoni @derberg
Changed variable to constant
c9fc51e 
Co-Authored-By: Lukasz Gornicki <[email protected]>
@jonaslagoni @derberg
Update lib/models/asyncapi.js
ac16304 
Co-Authored-By: Lukasz Gornicki <[email protected]>
@jonaslagoni
Copy link
Member Author

jonaslagoni commented Apr 3, 2020 •
edited
Loading

Thanks for the review!

  • extend should return an array with all the schemas used in the document with "nested json schema" case we discussed under the issue. or maybe a separate test?

Agree, however this PR only tackles the problem that the id of recursive schemas are never sat, i'll add this test in the other PR 😄

  • would be also great so see some tests that makes sure assignUidToParameterSchemas works as expected

I agree, I'll add this now.

@jonaslagoni jonaslagoni requested a review from derberg April 4, 2020 17:26
@derberg
Copy link
Member

derberg commented Apr 8, 2020

sorry @jonaslagoni that you had to ping me again and instead of approval I'm again back with a question, but didn't we agree in the issue that we should not assume id to be the same as the property key "x-parser-schema-id": "streetlightId" but all should be anonymous?

@jonaslagoni
Copy link
Member Author

@derberg no worries 😄 Yes, and I fail to see where the properties are being assigned to the property name instead of anonymous id?

The current implementation should:

  1. Assign anonymous id's recursively for all schemas regardless of it being a property in an object (which is as discussed in the issue).
  2. Assign parameter names as ids which is what the new test checks. We do this for the same reason schema and message components gets their names assigned as ids.

@derberg
Copy link
Member

derberg commented Apr 8, 2020

@jonaslagoni you are so right man, I should limit 🥃 in the late evenings

derberg
derberg previously approved these changes Apr 8, 2020
View reviewed changes
@jonaslagoni
Copy link
Member Author

@derberg added your last changes 😄

@derberg
Copy link
Member

derberg commented Apr 9, 2020

:shipit:

@derberg derberg changed the title Fixing missing anonymous schema id's fix: add missing anonymous schema id's Apr 9, 2020
@derberg derberg merged commit 5328fb3 into asyncapi:master Apr 9, 2020
@asyncapi-bot
Copy link
Contributor

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

@derberg derberg derberg approved these changes

@fmvilas fmvilas Awaiting requested review from fmvilas

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
Make JS parser stable
Development

Successfully merging this pull request may close these issues.

Schemas are not assigned correct uid's
4 participants
@jonaslagoni @derberg @asyncapi-bot @fmvilas