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

resolve Ref in resolveSchema #2001

Merged
merged 1 commit into from
Aug 19, 2022
Merged

resolve Ref in resolveSchema #2001

merged 1 commit into from
Aug 19, 2022

Conversation

LukasBoll
Copy link
Contributor

No description provided.

sdirix
sdirix requested changes Aug 19, 2022
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I understand the concept you were going for but I would keep the changes much more narrow in scope. Instead of modifying the core resolving functionality I would like to suggest to just trigger it again for the currently failing case (similar to depth = 1).

This would mean just triggering the resolve again in this line in case the schema is just a ref. Then the other changes are not needed.

I added some comments to the code, but as indicated I would expect all the current changes to be reverted so you don't need to act on them.

packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved
packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved
packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved
packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved
@sdirix sdirix removed the request for review from lucas-koehler August 19, 2022 09:05
@LukasBoll LukasBoll changed the title WIP: resolve Ref in resolfeSchema resolve Ref in resolfeSchema Aug 19, 2022
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.356% when pulling 7eba860 on LukasBoll:indexOfFittingSchema into 73142a3 on eclipsesource:master.

@sdirix sdirix changed the title resolve Ref in resolfeSchema resolve Ref in resolveSchema Aug 19, 2022
sdirix
sdirix approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Nice! That's how I wanted to see it! Great!

@sdirix sdirix merged commit 69441cd into eclipsesource:master Aug 19, 2022
@sdirix sdirix linked an issue Aug 19, 2022 that may be closed by this pull request
indexOfFittingSchema for oneOf doesn't work anymore if $ref is used #1991
Closed
@sdirix sdirix mentioned this pull request Aug 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdirix sdirix sdirix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

indexOfFittingSchema for oneOf doesn't work anymore if $ref is used
3 participants
@LukasBoll @coveralls @sdirix