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(codec): Parse numbers in arrays correctly #365

Merged
merged 2 commits into from
Jun 13, 2019

Conversation

kattrali
Copy link
Contributor

@kattrali kattrali commented Jun 12, 2019

Goal

Fix parsing logic for number values within arrays

Design

Previously, parsing numbers from JSON arrays exited prematurely if the "name" value was empty. However, name is always empty when parsing arrays, and is only used when parsing JSON objects.

So instead of exiting early to prevent parsing an invalid name, pass nil along as the name value.

Tests

  • Added a new test for adding metadata array
  • Enabled some unused (??) tests for JSON parsing

Review

  • This pull request is ready for:
    • Final review
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

kattrali and others added 2 commits June 12, 2019 16:45
remove duplicate NULL check as stringFromCString already handles NULL
@paulz paulz self-requested a review June 12, 2019 18:20
Copy link
Contributor

@paulz paulz left a comment

Choose a reason for hiding this comment

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

Great to see uncommented tests. Thank you!

Just made a small change to remove extra NULL check.

// XCTAssertNil(error, @"");
// XCTAssertEqualObjects(result, original, @"");
//}
- (void)testSerializeDeserializeArrayMultipleEntries
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome job restoring commented out tests!

@@ -1,17 +1,20 @@
Feature: Handled Errors and Exceptions

Scenario: Override errorClass and message from a notifyError() callback and discard lines from stack
Scenario: Override errorClass and message from a notifyError() callback, customize report
Copy link
Contributor

Choose a reason for hiding this comment

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

proposing adding the comment to the scenario about metadata, please adjust wording as necessary.

@kattrali kattrali merged commit 62ec38e into master Jun 13, 2019
@kattrali kattrali deleted the kattrali/fix-json-parsing branch June 13, 2019 11:35
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.

2 participants