Skip to content

Use with_reporter() internally#355

Merged
krlmlr merged 6 commits into
r-lib:masterfrom
krlmlr:with_reporter
Feb 19, 2016
Merged

Use with_reporter() internally#355
krlmlr merged 6 commits into
r-lib:masterfrom
krlmlr:with_reporter

Conversation

@krlmlr
Copy link
Copy Markdown
Member

@krlmlr krlmlr commented Jan 27, 2016

for better control over the testing logic (e.g., stop if encountering any error: #308).

In the long run, I think the code that's now in test_file() should be vectorized to allow for several files (perhaps extract the logic to a new function test_file_one()). Then test_files() simply calls test_file().

Kirill Müller added 2 commits January 27, 2016 00:52
so that end_reporter() is called in one place only
Comment thread R/test-files.r
results <- lapply(paths, test_file, env = env,
reporter = current_reporter, start_end_reporter = FALSE)
current_reporter$end_reporter()
with_reporter(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this would be clearer as results <- with_reporter(current_reporter, ...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

with_reporter() returns the reporter, not the results of the code. At least one package (checkmate) depends on this behavior. I'll add documentation.

@hadley
Copy link
Copy Markdown
Member

hadley commented Feb 18, 2016

Otherwise, looks good.

@krlmlr
Copy link
Copy Markdown
Member Author

krlmlr commented Feb 18, 2016

Documentation added.

@hadley
Copy link
Copy Markdown
Member

hadley commented Feb 19, 2016

LGTM. Can you please add a news bullet and merge?

krlmlr added a commit that referenced this pull request Feb 19, 2016
- `with_reporter()` is used internally and gains new argument `start_end_reporter = TRUE`.
@krlmlr krlmlr merged commit c068dc4 into r-lib:master Feb 19, 2016
@krlmlr krlmlr deleted the with_reporter branch February 19, 2016 08:59
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