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

Fix HasChange when using nested schema.Set #362

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

obourdon
Copy link
Contributor

This fixes #361

@paultyng
Copy link
Contributor

@obourdon this seems like a reasonable fix, do you mind adding some testing around the issue to demonstrate the problems (and prevent regression)?

Is it possible the AWS code should be modified to use HashEqual instead? Currently the way the SDK does most set handling is via the hash, this is changing a bit in 0.12 but will probably remain this way in this version of the SDK for the time being.

@obourdon
Copy link
Contributor Author

@paultyng many thanks for the comments above. Will definitely look into putting some tests and also have a look at your suggestion about AWS HashEqual. I just fixed the formatting as I did not see that the CI had failed. Many thanks again

@paultyng
Copy link
Contributor

The more I thought about this, I think this approach to Equal is your best bet, as HasChange has different mean than the set hashing which is mostly about matching up values for diffing. Will have someone take a peek, but we'll need some test coverage to merge.

@obourdon
Copy link
Contributor Author

@paultyng test has been added and I have verified that reverting commit
48bd90d fails and succeed otherwise running:

go test ./... -run 'TestSet.*' -count=1

@kmoe kmoe added this to the v2.0.0 milestone Apr 23, 2020
@appilon appilon removed this from the v2.0.0 milestone Apr 23, 2020
@appilon appilon changed the base branch from master to v1-maint April 23, 2020 18:26
@appilon
Copy link
Contributor

appilon commented Apr 23, 2020

We have recently pushed V2's history to master, V2 is now the mainline of development, I am retargeting your PR to v1-maint. Apologies for any inconvenience, for more information on V2 of the SDK, please see our discuss post

@appilon
Copy link
Contributor

appilon commented Apr 23, 2020

☝️ That was a stock message. Sorry about the delay on this PR, I will try and take a look at this very soon, it may need to land in V2 if it causes some kind of breaking change.

@obourdon
Copy link
Contributor Author

@appilon no pb at all, just let me know if you need me for anything.

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for contributing!

@ghost
Copy link

ghost commented May 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HasChange wrongly returns TRUE on nested shema.Set structures
5 participants