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

Follow up fix to #595 not to delete other hashes during serialization. #598

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

dnkoutso
Copy link
Contributor

@dnkoutso dnkoutso commented Oct 3, 2019

No description provided.

@dnkoutso dnkoutso added this to the 1.8.3 milestone Oct 3, 2019
@dnkoutso dnkoutso changed the title Follow up fix to [#595](https://github.com/CocoaPods/Core/pull/595) not to delete other hashes during serialization. Follow up fix to #595] not to delete other hashes during serialization. Oct 3, 2019
Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

Needs a test for the case this is trying to fix

lib/cocoapods-core/specification/json.rb Outdated Show resolved Hide resolved
@dnkoutso
Copy link
Contributor Author

dnkoutso commented Oct 3, 2019

The test that was written in #595 still passes.

@dnkoutso
Copy link
Contributor Author

dnkoutso commented Oct 3, 2019

Verified that CocoaPods integration specs all pass with this change with 0 checksum changes to files.

@segiddins
Copy link
Member

The test that was written in #595 still passes.

but we should still add a new test for the bug this PR is trying to fix

@dnkoutso dnkoutso changed the title Follow up fix to #595] not to delete other hashes during serialization. Follow up fix to #595 not to delete other hashes during serialization. Oct 4, 2019
@@ -80,6 +81,15 @@ module Pod

new_json.should.equal json
end

it 'maintains correct order of keys across versions' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@segiddins this test fails when I add hash.delete('platforms') either inside or outside the conditional.

@dnkoutso dnkoutso merged commit 8ebf4b8 into CocoaPods:1-8-stable Oct 4, 2019
@dnkoutso dnkoutso deleted the json_follow_up_Fix branch October 4, 2019 18:06
@justinseanmartin
Copy link
Contributor

I still feel like making an entirely new hash will be less error prone for future changes.

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