Skip to content

Adds key mapping support for use in destination plugins. #38

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

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

bsneed
Copy link
Contributor

@bsneed bsneed commented Aug 12, 2021

  • Adds mapKeys function to JSON object w/ optional value transform step.
  • Adds tests for key mapping functionality.


// MARK: - Helpers

extension Dictionary where Key == String, Value == Any {
internal func mapKeys(_ keys: [String: String], valueTransform: ((_ key: Key, _ value: Value) -> Any)? = nil) throws -> [Key: Value] {
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 = nil on there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, cuz I want it to be optional. They likely just want to map keys, at which point, the values won't be transformed.

if !(newValue is [Key: Value]), let transform = valueTransform {
// it's not a dictionary apply our transform.

// note: if it's an array, we've processed any dictionaries inside
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

return newValue
}).dictionaryValue

XCTAssertTrue(output!["AKey1"] as! Int == 11)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change these to as? so we fail the unit test instead of crash the unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could I guess, though that doesn't really bother me. We'd need to go hit a bunch of other tests as well as that's my MO for unit tests. FORCE UNWRAP ALL THE THINGS!

Copy link
Contributor

@migs647 migs647 left a comment

Choose a reason for hiding this comment

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

LGTM -- just some nitpicks

Copy link
Collaborator

@alanjcharles alanjcharles left a comment

Choose a reason for hiding this comment

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

looks great. Awesome idea.

@bsneed bsneed merged commit dc0e797 into main Aug 12, 2021
@bsneed bsneed deleted the bsneed/mapping branch August 12, 2021 22:58
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