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

[Bug Fix][Amplify API] Fix the bug which fails to update the value to nil #519

Merged
merged 5 commits into from
Jun 4, 2020

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Jun 4, 2020

Description of changes:
This PR contains one line of code change, but fix the bug which fails to update the value to nil.

dict[key] = nil removes the key from the dict
https://developer.apple.com/documentation/swift/dictionary/2885650-subscript

dict.updateValue(nil, forKey: key) updates the value to nil without removing the key
https://developer.apple.com/documentation/swift/dictionary/3127179-updatevalue

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

Overall lgtm, just a couple of requests:

  • add a test to verify nil is getting added correctly to the input dictionary (to avoid regressions in the future)
  • add some context about the issue in the PR description. Brief description of dict[key] = nil vs dict.updateValue(nil, forKey: key) with a link to the main docs

@ruiguoamz ruiguoamz requested a review from lawmicha June 4, 2020 19:30
Podfile.lock Outdated
COCOAPODS: 1.9.3
COCOAPODS: 1.9.2
Copy link
Contributor

@lawmicha lawmicha Jun 4, 2020

Choose a reason for hiding this comment

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

if 1.9.3 is already checked in, let's stick with that and upgrade ur local cocoapods version to 1.9.3 to avoid downgrades every time, then when u do a pod install, this should be updated back to 1.9.3 and the change will be reverted after you commit/push

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

//

import XCTest
@testable import Amplify
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if Amplify is required in the tests below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked, Temporal isn't working without Amplify

XCTAssertEqual(graphQLInput["status"] as? String, status.rawValue)

XCTAssertTrue(graphQLInput.keys.contains("updatedAt"))
XCTAssertNil(graphQLInput["updatedAt"]!)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need the force unwrap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, graphQLInput["updatedAt"] is an Any??. But XCTAssertNil() accepts Any?. So I need to unwrap a layer.

Copy link
Contributor

@drochetti drochetti left a comment

Choose a reason for hiding this comment

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

ship it! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Issues related to the API category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants