Skip to content

Conversation

@ricokahler
Copy link
Contributor

@ricokahler ricokahler commented Jun 4, 2025

This PR refactors the main diffItem function to improve its control flow and readability. The changes make the logic for handling different data type comparisons more explicit and streamlined.

Key Changes:

  • Early Exit for Equality:
    • The function now immediately returns if source === target at the very beginning.
  • Dedicated Type Handlers:
    • The logic for determining aType, bType, aIsUndefined, bIsUndefined, dataType, and isContainer has been removed.
    • Instead, diffItem now has explicit checks and calls to dedicated handlers for common type pairs:
      • if (typeof source === 'string' && typeof target === 'string') calls diffString(...).
      • if (Array.isArray(source) && Array.isArray(target)) calls diffArray(...).
      • if (isRecord(source) && isRecord(target)) calls diffObject(...). (Introduced isRecord type guard).
  • Simplified Primitive/Fallback Handling:
    • Removed the diffPrimitive function. String diffing is now handled by diffString.
    • The logic for other primitive types or type mismatches is now handled by fallbacks:
      • If target === undefined, an unset patch is pushed.
      • Otherwise (implying a type change or a primitive value change other than string-to-string), a set patch with target is pushed.
  • Removed PrimitiveValue Type: The PrimitiveValue type alias is no longer needed as specific type checks are used.

Rationale:

  • Improved Readability: The new control flow is more direct and easier to follow. Instead of complex type determination upfront, it checks for specific common scenarios (string-to-string, array-to-array, object-to-object) and then handles the remaining cases (target is undefined, or a general set/replacement).
  • Clearer Type Handling: Explicitly calling diffString, diffArray, and diffObject makes the intent clearer for these common structured types.
  • Reduced Complexity: Eliminating intermediate type variables (aType, bType, etc.) and the diffPrimitive function simplifies the overall structure of diffItem.
  • No Functional Change in Output: These changes are purely a refactor of the control flow and do not alter the diffing logic or the generated patches themselves. The test suite continues to pass, confirming this.

This refactoring makes the core diffing logic within diffItem cleaner and more maintainable.

@ricokahler ricokahler force-pushed the v6_key-based-reordering branch from a40879b to f7bcff7 Compare June 4, 2025 15:32
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from d022678 to 5c829d6 Compare June 4, 2025 15:32
@ricokahler ricokahler marked this pull request as ready for review June 4, 2025 22:09
@ricokahler ricokahler requested a review from rexxars June 4, 2025 22:24
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from 5c829d6 to e8850b4 Compare June 6, 2025 21:57
@ricokahler ricokahler force-pushed the v6_key-based-reordering branch from f7bcff7 to 131e5bd Compare June 6, 2025 21:57
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from e8850b4 to e763f90 Compare June 6, 2025 22:00
@ricokahler ricokahler force-pushed the v6_key-based-reordering branch 2 times, most recently from 8a9f9ed to efa4722 Compare June 6, 2025 22:16
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from e763f90 to 3c0b6a7 Compare June 6, 2025 22:16
@ricokahler ricokahler changed the base branch from v6_key-based-reordering to graphite-base/43 June 13, 2025 19:37
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from 3c0b6a7 to 0c5fd70 Compare June 13, 2025 19:39
@graphite-app graphite-app bot changed the base branch from graphite-base/43 to main June 13, 2025 19:40
Replace complex nested type checking with straightforward early return
pattern for cleaner and more readable diff logic.

- Use early returns for string, array, and object type handling
- Remove diffPrimitive function and inline string diffing logic
- Simplify getDiffMatchPatch to only handle string types
- Add isRecord helper for cleaner object type checking
- Eliminate complex branching in favor of sequential type checks
@ricokahler ricokahler force-pushed the v6_simplify-diff-item branch from 0c5fd70 to 5fe4030 Compare June 13, 2025 19:40
@ricokahler ricokahler merged commit b367cbd into main Jun 13, 2025
7 checks passed
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