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

Circular data & cross references #110 #148

Merged
merged 1 commit into from Oct 8, 2014
Merged

Conversation

loamhoof
Copy link

@loamhoof loamhoof commented Oct 7, 2014

Adding an option (checkDuplicates) to catch circular and cross
references, disabled by default so as not to slow down any existing code (only a few bits of code are still executed when the option is disabled).

Implemented following @puzrin suggestion that is: do a first pass to get the duplicates, and then put the aliases inside the algorithm. Slower than @larkox 's implementation, but perhaps easier to maintain.

Also added a related test.

NB: There may be some unused aliases if a duplicate is found inside a custom type, which must be a pretty rare case.

if (0 !== index) {
_result += ', ';
if (!duplicate || !state.usedDuplicates[duplicateIndex]) {
state.usedDuplicates[duplicateIndex] = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you duplicate this code across all of the writers? Can it be placed in writeNode function?

Copy link
Author

Choose a reason for hiding this comment

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

I'll study the possibility.

@dervus
Copy link
Collaborator

dervus commented Oct 8, 2014

In conclusion, please merge all these commits together.


if (null !== object && 'object' === typeof object) {
index = objects.indexOf(object);
if (index !== -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, by the way, if you swap the operands here, it'll be great.

@loamhoof
Copy link
Author

loamhoof commented Oct 8, 2014

Sorry about the small remainders, should have seen them. Anyway, here you go.

@puzrin
Copy link
Member

puzrin commented Oct 8, 2014

https://github.com/nodeca/js-yaml/pull/148/commits

@loamhoof squash to single commit please. See 'git rebase --interactive' in google

Catch circular and cross references.
The object is first scanned to get these references before starting the writing algorithm. Aliases are then added during it.
@loamhoof
Copy link
Author

loamhoof commented Oct 8, 2014

@puzrin sorry about that, it's done.

dervus added a commit that referenced this pull request Oct 8, 2014
Circular data & cross references #110
@dervus dervus merged commit 2c23e7b into nodeca:master Oct 8, 2014
@dervus
Copy link
Collaborator

dervus commented Oct 8, 2014

Perfect! Thank you for contribution!

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.

None yet

3 participants