Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recycle brief if validation expands into multiple steps #564

Merged
merged 31 commits into from
Jan 22, 2025

Conversation

yjunechoe
Copy link
Collaborator

@yjunechoe yjunechoe commented Aug 19, 2024

This PR aims to fix #478 by recycling brief when possible.

# Single string `brief` recycles
create_agent(small_table) |> 
  col_exists(c("a", "b"), brief = "one") |> 
  el("validation_set") |> 
  el("brief")
#> [1] "one" "one"

# Multi-length `brief` applies if exactly matched in number of steps produced
create_agent(small_table) |> 
  col_exists(c("a", "b"), brief = c("one", "two")) |> 
  el("validation_set") |> 
  el("brief")
#> [1] "one" "two"

# Errors if not matched in length (columns x segments produces 6 steps here)
create_agent(small_table) |> 
  col_vals_equal(
    c("a", "b"), 0,
    segments = vars(f),
    brief = c("one", "two")
  )
#> Error in `resolve_briefs()`:
#> ! `brief` must be length 1 or 6, not 2

This is a draft PR until I:

  • Figure out the right level of abstraction for the resolve_brief() helper - namely, should it stand on its own (as current) or wrap over the default autobrief behavior, to host all brief-related logic? (will move forward with a unified resolve_brief() function)
  • Do the necessary copy-pastes for all user-facing validation functions
  • Write (batch?) tests
  • Ensure yaml writing and round-tripping (similar to 75767a4)

Out of scope but worth considering for afterwards is whether we should support glue sytnax here as well.

@yjunechoe yjunechoe marked this pull request as draft August 19, 2024 14:22
@rich-iannone
Copy link
Member

@yjunechoe just a note for future work on this PR: #579 changed settings in .Rproj to strip trailing whitespace. Merging from main and re-saving these files should fix the merge conflict.

@yjunechoe
Copy link
Collaborator Author

Got it - thanks for the heads up!

@yjunechoe yjunechoe marked this pull request as ready for review January 21, 2025 21:39
@yjunechoe
Copy link
Collaborator Author

@rich-iannone I think this is (finally) ready for review! The PR fixes the problem of brief failing to recycle across expanded steps. As a side effect, validation functions now generally support multi-length briefs, including a custom vector of briefs.

Internal details

The PR introduces resolve_brief() which handles the vector recycling as well as fallbacking to generate_autobriefs().

Batch tests are added to test-brief.R using the test_multi_briefs() helper, which will also print the agent report when ran interactively, for manual inspection of briefs.

TODO

I was thinking I should break up the PR as it's getting big. The one missing piece from this PR is supporting multi-length brief on the yaml read/write side of things. I plan to tackle this in a follow-up PR after this is merged.

I mentioned the possibility of supporting {glue} syntax in brief, like how we have it in label. If this is desirable, I think it should be simple to implement given the work in this PR, so this can be separately tackled in the near-term as well.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@rich-iannone
Copy link
Member

This is great, thanks for all the work you put into it! Will merge this now.

@rich-iannone rich-iannone merged commit a1ab25f into rstudio:main Jan 22, 2025
12 checks passed
@yjunechoe yjunechoe deleted the brief-recycle branch January 23, 2025 03:00
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.

For steps applied several times, personalized brief is only accessible for the first column
2 participants