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

[Config] 'includes' affect equality. #255

Merged
merged 1 commit into from
Apr 1, 2015
Merged

[Config] 'includes' affect equality. #255

merged 1 commit into from
Apr 1, 2015

Conversation

brianpartridge
Copy link

I was running into issues where I would load an xcconfig from disk, edit includes and try to save, but the change would not actually be saved. The issue was the equality check in save_as. Since equality was based on to_hash, if there were only changes that didn't affect the hash they would never be saved.
🌈

@segiddins
Copy link
Member

Why not just add includes to the comparison in ==?

@brianpartridge
Copy link
Author

I liked that this keeps the equality test simple without multiple respond_to checks. Using to_s does more compute work, but it's not significantly more. If we were going to go the route of using to_hash and includes, I would argue that we should be comparing attributes and other_linker_flags rather than to_hash because of the work that it does. Thoughts?

@segiddins
Copy link
Member

This seems fine to me. @mrackwitz ?

@mrackwitz
Copy link
Member

I agree with:

I would argue that we should be comparing attributes and other_linker_flags rather than to_hash

Basing the implementation of == on to_hash seemed to be convenient for the tests, but when we call it there explicitly anyway, then I'd pledge for implementing it properly.

@brianpartridge
Copy link
Author

@mrackwitz done.

@mrackwitz
Copy link
Member

Looks good! Thanks! 👍

mrackwitz added a commit that referenced this pull request Apr 1, 2015
@mrackwitz mrackwitz merged commit 6b4a41c into CocoaPods:master Apr 1, 2015
mrackwitz added a commit that referenced this pull request Apr 1, 2015
@brianpartridge
Copy link
Author

Sweet, thanks.

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