Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Oct 7, 2025

And add an explicit test for the behaviour

And add an explicit test for the behaviour
@claude
Copy link

claude bot commented Oct 7, 2025

Claude finished @hadley's task —— View job


Review Progress

  • Read changed files and understand modifications
  • Analyze changes for potential bugs or issues
  • Provide review feedback

Review Feedback

The PR fixes a typo in paste0() calls where collpase was incorrectly used instead of collapse. However, this fix reveals that the original intent was likely wrong:

R/expect-constant.R:89 and R/expect-equality.R:154: The change from:

paste0(comp, collpase = "\n")  # typo - would be ignored

to:

paste0(comp, "\n")  # works but different behavior

The original code had a typo in the collapse argument name, so it was being ignored and paste0() was concatenating each element of comp with "\n" individually. The fix removes the (misspelled) collapse argument entirely.

Potential issue: If comp is a vector with multiple elements, the original intended behavior (with collapse = "\n") would join all elements into a single string separated by newlines, while the current fix creates separate strings.

The test additions properly capture the expected behavior, so if the current output format is intentional, this is fine. But if the original intent was to collapse multiple comparison results into a single string, the fix should be:

paste0(comp, collapse = "\n")

LGTM if the current multi-line output format is intended.

@hadley hadley merged commit 63b7549 into main Oct 7, 2025
13 checks passed
@hadley hadley deleted the correct-paste-argument branch October 7, 2025 20:42
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