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

ArrayNodeDeserializer can now deserialize circular references. #937

Closed
wants to merge 1 commit into from

Conversation

MetaFight
Copy link
Contributor

@MetaFight MetaFight commented Jun 19, 2024

Resolves #933

The ArrayNodeDeserializer delegates the deserialization of elements to the CollectionNodeDeserializer.DeserializeHelper. This, in turn, handles circular references by relying on the IValuePromise.ValueAvailable event to write all the resolved references to its IList result parameter.

However, because ArrayNodeDeserializer doesn't know the size of its resulting array beforehand, it uses a temporary ArrayList which it passes to CollectionNodeDeserializer.DeserializeHelper. As a result, all resolved ValuePromise values are written to the temporary ArrayList instead of the final Array.

This PR fixes this by adding an optional Action<int, object?>? promiseResolvedHandler to CollectionNodeDeserializer.DeserializeHelper. When provided, it is used when ValuePromises are resolved. Otherwise, the existing behaviour is preserved.

…mises was being applied to the wrong object.
@MetaFight
Copy link
Contributor Author

I can't figure out why git thinks the entirety of the unit test file has changed. I've tried switching the line endings but I think in doing that (amending the commit and using push --force) I broke the CI build.

I'll have some time tomorrow to try and fix this.

@EdwardCooke
Copy link
Collaborator

The reason the CI failed is your branch name, when the PR branch is master the build fails.

@MetaFight
Copy link
Contributor Author

I'm abandoning this PR because of the branch name issue. I'll create a new one with a better branch name.

@MetaFight MetaFight closed this Jun 20, 2024
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.

Aliases do not resolve correctly in Sequences
2 participants