Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Oct 6, 2025

@DavisVaughan could you please have a look at the specific labels, but also contemplate what you think of this design.

@hadley hadley requested a review from DavisVaughan October 6, 2025 21:35
@claude
Copy link

claude bot commented Oct 6, 2025

Claude finished @hadley's task —— View job


Code Review Analysis

Todo List:

  • Read and analyze changed R source files
  • Review snapshot test changes
  • Identify potential bugs or issues
  • Provide concise feedback on design approach

Issues Found

Bug in expect_waldo_equal_() (R/expect-equality.R:154):

paste0(comp, collpase = "\n")  # typo: should be "collapse"

Potential issue in labelling approach:
The change from "names of " + act$lab to just act$lab in error messages may be less clear. Compare:

  • Before: Expected names of 'x' to have the same values as names of 'y'.
  • After: Expected 'x' to have the same names as 'y'.

The new approach is more concise but potentially ambiguous when the expectation is about names specifically.

Design Assessment

The refactoring improves consistency by making message templates explicit parameters rather than hardcoded strings. This makes the helper functions more flexible and reusable.

Copy link
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

I think more succinct is always better if it captures the same intent, and this seems to do that well 👍

@hadley hadley merged commit 3cb0cdc into main Oct 7, 2025
13 checks passed
@hadley hadley deleted the better-labels branch October 7, 2025 20:12
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