Skip to content

#226: fix for external-from-external dependencies (exposed by addition of dereferencedCache) #230

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

Closed

Conversation

mummybot
Copy link

This change resolves bug #226 and the PRs #146 and #223.

The if/then flow in the crawl functions of bundle and particularly dereference were such that if a match was found it would stop processing additional properties which may also contain $refs to be dereferenced.

@mummybot
Copy link
Author

mummybot commented May 4, 2021

Sorry to poke, but will this PR be looked at? Are there changes that I need to make, or is this outside of the structure of what json-schem-ref-parser will parse?

@philsturgeon philsturgeon requested a review from P0lip May 4, 2021 18:51
@philsturgeon
Copy link
Member

@mummybot
Copy link
Author

It highlights that a property sibling adjacent to a $ref doesn't get crawled and parsed currently. $bespokeKey is a representation of a custom keyword as allowed by Ajv, but it equally applies to the not property, which is valid JSON schema; we use a $ref for including long enum arrays.

@philsturgeon
Copy link
Member

Sorry I'm trying to figure out the problems with GitHub Actions, I might have just done it, but we've got some conflicts now. Could you have a look?

@mummybot
Copy link
Author

mummybot commented Jun 7, 2021

I'll fix the conflicts tomorrow.

@mummybot
Copy link
Author

mummybot commented Jun 8, 2021

Conflicts resolved @philsturgeon

@mummybot
Copy link
Author

Sorry to poke, any more movement on this PR? We are currently blocked from upgrading json-schema-ref-parser until this is merged.

@philsturgeon
Copy link
Member

@P0lip could I get your input on this one? We've fixed the status checks, and there are no more conflicts, just need to figure out if this is something we want to merge.

@mummybot
Copy link
Author

@P0lip sorry for chasing, but is this something which you will consider?

@mummybot
Copy link
Author

Sorry to bug again, this is sitting in our backlog as a blocker and I have to justify it every morning. @P0lip / @philsturgeon is this ever going to be looked at, or shall I just close it as a "won't fix"?

@mummybot
Copy link
Author

@philsturgeon will this PR be reviewed and maybe merged? We still have it outstanding in our work backlog. If not, then fine and I will close it.

@philsturgeon
Copy link
Member

philsturgeon commented May 11, 2022

Hey, I know this has been going on for ages but I've never really understood what is happening in these tests.

What is $bespokeKey? are we adding some custom JSON Schema logic, or are you picking names that look like special characters with the intention of making sure keys with $ at the start still work?

The tests are just a bit confusing but it looks like a lot of effort has gone in here. Now that I've cleared some bugs and got the release pipeline working I am interested to get this sorted ASAP and will keep an eye out for updates.

@jonluca
Copy link
Collaborator

jonluca commented Sep 18, 2023

I'm closing stale PRs - happy to merge in a new PR if it's rebased

@jonluca jonluca closed this Sep 18, 2023
@mummybot
Copy link
Author

God, it is so long ago that I looked at this, and I missed @philsturgeon's last message back in May 2022 :( I've no idea if this is still an issue, but the rebase is now horrendous, and I've moved projects.

If I encounter this issue again on a new project, I'll pick this up. A shame it didn't get added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants