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

Bound recursive checksumming #518

Merged
merged 1 commit into from
Jan 11, 2015

Conversation

denisdefreyne
Copy link
Member

Fix for #469. Instead of preventing items and layouts in attributes, we can allow them and let the checksummer properly deal with it.

PR #507 had a different implementation, but too much like a hack.

digest.update(obj.class.to_s)

if visited.include?(obj)
digest.update('recur')
Copy link
Member

Choose a reason for hiding this comment

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

if visited would be a hash then we could store also the digest value and set it here, would it be useful at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can’t set the digest value because at the time it isn’t known yet.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see it now, painful

@denisdefreyne
Copy link
Member Author

Can I get a +1?

@mpapis
Copy link
Member

mpapis commented Jan 11, 2015

looks good but the jruby test failed, I have restarted it to check if it was one time or an real problem

@denisdefreyne
Copy link
Member Author

Tests are passing on Travis CI, but GitHub isn’t picking up the success. Odd!

denisdefreyne added a commit that referenced this pull request Jan 11, 2015
@denisdefreyne denisdefreyne merged commit e3b0740 into release-3.7.x Jan 11, 2015
@denisdefreyne denisdefreyne deleted the bounded-recursive-checksumming branch January 11, 2015 11:22
denisdefreyne added a commit that referenced this pull request Jan 11, 2015
denisdefreyne added a commit that referenced this pull request Jan 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants