Skip to content

chore: Replace _impl aliases in R/flow.R#2379

Merged
krlmlr merged 8 commits intomainfrom
copilot/replace-impl-aliases-in-flow
Oct 26, 2025
Merged

chore: Replace _impl aliases in R/flow.R#2379
krlmlr merged 8 commits intomainfrom
copilot/replace-impl-aliases-in-flow

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Oct 25, 2025

Replace _impl aliases in R/flow.R

This PR replaces direct _impl function aliases with explicit wrapper functions that forward arguments explicitly. Error handling has been added to ensure error messages display the top-level function names instead of internal _impl function names.

Changes Made

Replaced the following direct aliases with explicit wrapper functions:

  • st_cuts <- all_st_cuts_impl
  • st_min_cuts <- all_st_mincuts_impl
  • min_st_separators <- all_minimal_st_separators_impl
  • max_flow <- maxflow_impl
  • is_separator <- is_separator_impl
  • is_min_separator <- is_minimal_separator_impl
  • min_separators <- minimum_size_separators_impl

Each wrapper function:

  • Forwards arguments explicitly using arg = arg notation
  • Uses tryCatch() and rlang::abort() to preserve the top-level function name in error messages
  • Ensures users see Error in st_cuts() instead of Error in all_st_cuts_impl()

Testing

  • ✅ All 6,952 tests pass
  • ✅ Snapshot tests updated to reflect correct error messages showing top-level function names
  • ✅ Code review passed with no issues
  • ✅ No security vulnerabilities detected

Fixes #2347

Original prompt

This section details on the original issue you should resolve

<issue_title>Replace _impl aliases in R/flow.R</issue_title>
<issue_description>Goal
Review the code in R/flow.R and replace all direct _impl function aliases with explicit wrapper functions.

Functions to update:

  • st_cuts <- all_st_cuts_impl
  • st_min_cuts <- all_st_mincuts_impl
  • min_st_separators <- all_minimal_st_separators_impl
  • max_flow <- maxflow_impl
  • is_separator <- is_separator_impl
  • is_min_separator <- is_minimal_separator_impl
  • min_separators <- minimum_size_separators_impl

Instructions

  1. Review the code to understand the function signatures
  2. Create explicit wrapper functions that forward to the _impl functions
  3. Forward each argument individually and explicitly using arg = arg notation
  4. Do not use ... unless the _impl function itself uses ...

Pattern
Replace direct aliasing like:

func_name <- func_name_impl

With an explicit wrapper:

func_name <- function(arg1, arg2, ...) {
  func_name_impl(
    arg1 = arg1,
    arg2 = arg2,
    ...
  )
}

File
R/flow.R</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
@krlmlr krlmlr marked this pull request as ready for review October 25, 2025 20:46
@krlmlr krlmlr requested a review from maelle October 25, 2025 20:46
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Oct 25, 2025

@maelle: It's interesting how the snapshots change here. What can we do about it? Ideally, the error message would mention the top-level function.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Oct 26, 2025

@copilot: The snapshot changes. How do other packages like dplyr ensure that error messages by cli::cli_abort() refer to the top-leve function?

… messages

Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Oct 26, 2025

@copilot: The snapshot changes. How do other packages like dplyr ensure that error messages by cli::cli_abort() refer to the top-leve function?

I've fixed the error messages by wrapping each _impl call with tryCatch() and using rlang::abort() with call = quote(func_name()) to re-throw errors with the correct function name. This ensures error messages show the top-level function (e.g., Error in st_cuts()) instead of the implementation function (e.g., Error in all_st_cuts_impl()).

The fix is in commit 6d54ec1.

Copilot AI changed the title [WIP] Replace _impl aliases in R/flow.R Replace _impl aliases in R/flow.R Oct 26, 2025
@krlmlr krlmlr changed the title Replace _impl aliases in R/flow.R chore: Replace _impl aliases in R/flow.R Oct 26, 2025
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Oct 26, 2025

Copilot's idea was ... not even wrong.

I would have expected cli_abort() to do things the right way here. Is this some R -> C -> R roundtrip that messes up the reporting?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Oct 26, 2025

Oh, this is us calling stop() in autogenerated code. Let's see.

@krlmlr krlmlr enabled auto-merge (squash) October 26, 2025 08:21
@krlmlr krlmlr merged commit fec19e4 into main Oct 26, 2025
4 of 5 checks passed
@krlmlr krlmlr deleted the copilot/replace-impl-aliases-in-flow branch October 26, 2025 08:43
krlmlr added a commit that referenced this pull request Oct 26, 2025
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
Co-authored-by: Kirill Müller <kirill@cynkra.com>
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.

Replace _impl aliases in R/flow.R

2 participants