Fix problems() support for vroom_lines() output (#607) - #624
Closed
LeonidasZhak wants to merge 2 commits into
Closed
LeonidasZhak wants to merge 2 commits into
LeonidasZhak wants to merge 2 commits into
Conversation
vroom_lines() extracts a character column from the tibble returned by vroom_(), discarding the 'problems' attribute. This commit propagates the attribute when parsing errors exist, and teaches problems() to accept character vectors with a 'problems' attribute. Fixes #607
The previous fix checked nrow(vroom_errors_(probs)) > 0 before propagating the problems attribute. However, errors are populated lazily during ALTREP vector materialization (element access), so the check always found 0 errors at return time. This fix propagates the problems attribute unconditionally when it exists, matching how vroom() always carries the problems pointer. The pointer is shared, so errors added during later materialization are visible when problems() is called. Also adds a regression test exercising the full round-trip: vroom_lines() with embedded null → problems() returns the errors.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| test_that("vroom_lines respects n_max", { | ||
| infile <- vroom_example("mtcars.csv") | ||
| expect_equal(vroom_lines(infile, n_max = 2), readLines(infile, n = 2)) | ||
| expect_equal(vroom_lines(infile, n_max = 2), readLines(infile, n = 2), ignore_attr = "problems") |
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| expect_equal(vroom_lines(infile, n_max = 2), readLines(infile, n = 2), ignore_attr = "problems") | |
| expect_equal( | |
| vroom_lines(infile, n_max = 2), | |
| readLines(infile, n = 2), | |
| ignore_attr = "problems" | |
| ) |
|
|
||
| test_that("vroom_lines uses na argument", { | ||
| expect_equal(vroom_lines(I("abc\n123"), progress = FALSE), c("abc", "123")) | ||
| expect_equal(vroom_lines(I("abc\n123"), progress = FALSE), c("abc", "123"), ignore_attr = "problems") |
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| expect_equal(vroom_lines(I("abc\n123"), progress = FALSE), c("abc", "123"), ignore_attr = "problems") | |
| expect_equal( | |
| vroom_lines(I("abc\n123"), progress = FALSE), | |
| c("abc", "123"), | |
| ignore_attr = "problems" | |
| ) |
| vroom::vroom_write(df, f, append = TRUE) | ||
|
|
||
| expect_equal(vroom_lines(f), c("x\ty", "1\t2", "1\t2")) | ||
| expect_equal(vroom_lines(f), c("x\ty", "1\t2", "1\t2"), ignore_attr = "problems") |
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| expect_equal(vroom_lines(f), c("x\ty", "1\t2", "1\t2"), ignore_attr = "problems") | |
| expect_equal( | |
| vroom_lines(f), | |
| c("x\ty", "1\t2", "1\t2"), | |
| ignore_attr = "problems" | |
| ) |
| vroom_write(data.frame(), file = tf, append = TRUE, delim = ",") | ||
|
|
||
| expect_equal(vroom_lines(tf, altrep = FALSE), c("a,b,c", "1,2,3")) | ||
| expect_equal(vroom_lines(tf, altrep = FALSE), c("a,b,c", "1,2,3"), ignore_attr = "problems") |
There was a problem hiding this comment.
[air] reported by reviewdog 🐶
Suggested change
| expect_equal(vroom_lines(tf, altrep = FALSE), c("a,b,c", "1,2,3"), ignore_attr = "problems") | |
| expect_equal( | |
| vroom_lines(tf, altrep = FALSE), | |
| c("a,b,c", "1,2,3"), | |
| ignore_attr = "problems" | |
| ) |
Author
|
Withdrawing this because CI is currently failing and I should not ask maintainers to review an unready automated PR. I will only come back with a clean, focused change if useful. Sorry for the noise, and thank you for maintaining the project. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #607.
Problem
When
vroom_lines()encounters a parsing issue, it warns users to callproblems(). However:vroom_lines()discards the"problems"attribute when extracting the character columnproblems()only acceptstbl_dfobjects, rejecting character vectorsFix
R/vroom_lines.R: Propagate the"problems"attribute from the tibble to the character vector when parsing errors existR/problems.R: Accept character vectors with a"problems"attribute; improve error messages for unsupported typesman/problems.Rd: Update@param xto document character vector supporttests/testthat/test-vroom_lines.R: Add tests for attribute propagation and error messagesValidation
The pre-existing failure at
test-problems.R:132is unrelated (vroom_materializenot found in test harness).