Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Oct 6, 2025

Reverts #2150 since it does actually break quite a few packages on CRAN. This change reverts the original behaviour (with more tests) making use of more new helpers.

Fixes #2245

hadley added 2 commits October 6, 2025 12:55
Reverts #2150 since it does actually break quite a few packages on CRAN. This change reverts the original behaviour (with more tests) making use of more new helpers.

Fixes #2245
@claude
Copy link

claude bot commented Oct 6, 2025

Claude finished @hadley's task —— View job


Code Review Analysis

Reviewing PR #2255 that reverts expect_mapequal() changes from #2150 due to CRAN package breakage.

Todo List:

  • Read and analyze changed files
  • Review the reversion logic in R/expect-setequal.R
  • Check test coverage and snapshots
  • Identify any potential bugs or issues
  • Provide final review feedback

Review Findings

The reversion looks correct and addresses the CRAN breakage issue appropriately:

Positive aspects:

  • expect_mapequal() now properly handles edition differences (lines 110-117 in R/expect-setequal.R)
  • Edition 2 behavior is preserved using expect_equal() for backward compatibility
  • Edition 3+ uses the newer expect_waldo_equal_() approach
  • Comprehensive test coverage including edge cases (empty vectors, validation, etc.)
  • Good input validation with check_map_names() helper function
  • Proper error messages with clear descriptions

Minor observations:

  • The warning message for empty vectors (object and expected are empty vectors.) uses backticks inconsistently with other error messages that use proper formatting
  • The code correctly preserves the original behavior that caused CRAN issues while maintaining the improvements

Technical correctness:

  • Edition-based behavior switching is implemented correctly
  • Name validation logic is sound
  • Test snapshots cover all expected failure cases
  • The reversion maintains API compatibility

LGTM - No significant issues found. The reversion appropriately addresses the CRAN breakage while maintaining test coverage and proper edition handling.


@hadley hadley merged commit e8b06e1 into main Oct 6, 2025
13 checks passed
@hadley hadley deleted the revert-mapequal branch October 6, 2025 19:59
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.

expect_mapequal() no longer works with vectors

2 participants