Refactor wildcard index logic by timkimadobe · Pull Request #416 · adobe/aepsdk-edge-ios

timkimadobe · 2023-10-03T01:09:39Z

Description

This PR:

Updates how wildcard indexes are allowed to be specified, only allowing a strictly validated format of a single wildcard character to the left of the index, ex: [*123]. If the wildcard candidate fails validation, it causes a test failure to help test writers and maintainers enforce consistency across usages
Updates JSON comparison public method docs to reflect this change
Updates relevant test case to validate new wildcard format and strict test failure enforcement
Splits the logic of extracting and validating the wildcard indexes from the flexible array comparison method, which has the benefits of:
1. Abstracting away the complexity of the string processing and validation
2. Moving the verbose documentation away from the business logic of the array comparison

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

…tion and replacement groups are not needed

codecov · 2023-10-03T01:17:38Z

Codecov Report

Merging #416 (c014f56) into feature/json-comparison-parity (b8dd273) will not change coverage.
The diff coverage is n/a.

@@                       Coverage Diff                       @@
##           feature/json-comparison-parity     #416   +/-   ##
===============================================================
  Coverage                           96.77%   96.77%           
===============================================================
  Files                                  27       27           
  Lines                                1671     1671           
===============================================================
  Hits                                 1617     1617           
  Misses                                 54       54

emdobrin · 2023-10-03T20:33:13Z

Tests/TestUtils/XCTestCase+AnyCodableAsserts.swift

+                // Discard wildcard indexes that are out of bounds of the available expected indexes
+                let intersection = expectedIndexes.intersection(result)
+                wildcardIndexes = intersection
+                // Remove all wildcard indexes from the valid expected indexes, so validation is not performed twice


Looks good. Just a small comment, some of these comments may be too verbose and it might be difficult to read through code. Can you extract this else block in a function with description and/or consolidate the comments in a single block?

Thank you! I tried consolidating into a single block at the start of the logic, what do you think?

addb · 2023-10-03T23:36:33Z

Tests/TestUtils/XCTestCase+AnyCodableAsserts.swift

+                    .filter { $0.contains("*") }
+                    .compactMap { Int($0.replacingOccurrences(of: "*", with: "")) })
+
+                let intersection = expectedIndexes.intersection(result)


nit: Update variable names with more context and add comments to make the code easy to read. Something on these lines:

// Filters all the entries with wildcard character `*` and returns the index marked with wildcard. var listOfIndexes = Set(indexValues .filter { $0.contains("*") } .compactMap { Int($0.replacingOccurrences(of: "*", with: "")) })

// result of regex matching with list of wildcard indexes let wildcardIndexes = expectedIndexes.intersection(result)

Updated variable names! As for the inline comments, they were inline before but updated to be collected together before the else block logic based on the discussion: #416 (comment)

@emdobrin would inline be clearer in terms of explaining the intent of each step?

addb · 2023-10-03T23:37:36Z

Tests/TestUtils/XCTestCase+AnyCodableAsserts.swift

+                // Discard wildcard indexes that are out of bounds of the available expected indexes
+                // Then remove all wildcard indexes from the valid expected indexes, so validation is not performed twice
+
                let arrayIndexValueRegex = #"^\[(.*?)\]$"#


nit: add comment
// regex to extract indexes with wildcard matching identifier '*'

timkimadobe · 2023-10-05T00:32:00Z

Based on offline discussion with @emdobrin and @addb, aligned on the following updates:

Extract the else block of logic that filters and processes wildcard indexes into its own method and simplify callsite
Refactor the logic for wildcard indexes to strictly validate the following cases, and emit a test error upon failure:
1. Wildcard character must be placed on the left side of the array integer, as the first character in the array brackets
2. The inner string must be parsable as a valid Int once the wildcard character is removed

With the consequences of

Abstracts away the more complicated logic of handling wildcard processing into its own module, simplifying the intent of the flexible array comparison method
Creates test case writing and maintaining consistency, where wildcard usage is standardized, and when encountered in a test case the intent is clear

Extract wildcard array index extraction logic into separate function Update usages, test cases, and method documentation

emdobrin

looks great

emdobrin · 2023-10-05T18:10:24Z

Tests/TestUtils/XCTestCase+AnyCodableAsserts.swift

+        var result: Set<Int> = []
+        for potentialWildcardIndex in potentialWildcardIndexes {
+            if potentialWildcardIndex.first != "*" {
+                XCTFail("TEST ERROR: wildcard indexes must have a single `*` character to the left of the index.", file: file, line: line)


nit: let's include a simple example for the implementor: ... to the left of the index, ie [*0].

That's a great idea! Thank you, updated to include example

* JSON comparison parity (utility only) (#401) * Remove custom helper enums and refactor usages Update documentation and code logic to match Android implementation * Apply lint autocorrect * Revert "Apply lint autocorrect" This reverts commit 7f38d19. * Apply lint autocorrect * Reduce wordiness of file and line param descriptions * Update documentation null to nil * Update file and line param descriptions * Add method documentation to all helper methods Update param name of extractArrayStyleComponents to be more intuitively descriptive * JSON comparison parity - test cases (#402) * Achieves parity with the Android Edge test setup for JSON comparison Splits coverage into parameterized and special case tests, and update provides more comprehensive systematic coverage * Add additional test cases to validate type mismatch detection and all boolean cases Update index helper comments based on changed values * Update failure cases to check for unintended type casting * Fix comment typo * Extract inline actual AnyCodable creation to separate line for clarity * Refactor wildcard index logic (#416) * Refactor wildcard index access to flatten logic since different condition and replacement groups are not needed * Reformat method documentation to bring them all together * Rename variables to better reflect what each one stores * Refactor wildcard array index extraction to strictly validate format Extract wildcard array index extraction logic into separate function Update usages, test cases, and method documentation * Update test failure message to include example format * Rename `extractArrayStyleComponents` and update method docs (#418) * Rename the extractArrayStyleComponents -> extractArrayFormattedComponents to better reflect actual behavior Update method documentation to reflect the rename and to better explain the logic * Update parameter description * Update method description * Include information about removing escape character

Refactor wildcard index access to flatten logic since different condi…

87acb19

…tion and replacement groups are not needed

timkimadobe requested review from addb, cacheung, emdobrin and kevinlind October 3, 2023 01:09

timkimadobe mentioned this pull request Oct 3, 2023

JSON comparison system parity #415

Merged

10 tasks

emdobrin reviewed Oct 3, 2023

View reviewed changes

Reformat method documentation to bring them all together

c88e7dc

timkimadobe requested a review from emdobrin October 3, 2023 20:54

addb reviewed Oct 3, 2023

View reviewed changes

Rename variables to better reflect what each one stores

225afe0

Refactor wildcard array index extraction to strictly validate format

e863cbf

Extract wildcard array index extraction logic into separate function Update usages, test cases, and method documentation

timkimadobe requested a review from addb October 5, 2023 01:32

emdobrin approved these changes Oct 5, 2023

View reviewed changes

Update test failure message to include example format

c014f56

timkimadobe changed the title ~~Refactor wildcard index access to flatten logic~~ Refactor wildcard index logic Oct 6, 2023

timkimadobe merged commit 7e89778 into adobe:feature/json-comparison-parity Oct 6, 2023

timkimadobe deleted the feature/json-comparison-parity branch October 6, 2023 00:15

timkimadobe restored the feature/json-comparison-parity branch October 6, 2023 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor wildcard index logic#416

Refactor wildcard index logic#416
timkimadobe merged 5 commits intoadobe:feature/json-comparison-parityfrom
timkimadobe:feature/json-comparison-parity

timkimadobe commented Oct 3, 2023 •

edited

Loading

Uh oh!

codecov bot commented Oct 3, 2023 •

edited

Loading

Uh oh!

emdobrin Oct 3, 2023

Uh oh!

timkimadobe Oct 3, 2023

Uh oh!

addb Oct 3, 2023

Uh oh!

timkimadobe Oct 3, 2023

Uh oh!

addb Oct 3, 2023 •

edited

Loading

Uh oh!

timkimadobe commented Oct 5, 2023

Uh oh!

emdobrin left a comment

Uh oh!

emdobrin Oct 5, 2023

Uh oh!

timkimadobe Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

timkimadobe commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

emdobrin Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

timkimadobe Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

addb Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

timkimadobe Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

addb Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timkimadobe commented Oct 5, 2023

Uh oh!

emdobrin left a comment

Choose a reason for hiding this comment

Uh oh!

emdobrin Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

timkimadobe Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timkimadobe commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 3, 2023 •

edited

Loading

addb Oct 3, 2023 •

edited

Loading