Skip to content

Reporters always write to true stdout#420

Merged
hadley merged 13 commits into
r-lib:masterfrom
krlmlr:feature/error-in-message-redir
Mar 23, 2016
Merged

Reporters always write to true stdout#420
hadley merged 13 commits into
r-lib:masterfrom
krlmlr:feature/error-in-message-redir

Conversation

@krlmlr
Copy link
Copy Markdown
Member

@krlmlr krlmlr commented Mar 22, 2016

Works by memoizing stdout() on creation, and writing strictly to the memoized connection (and not to the possibly overridden standard output).

> std <- stdout(); with_sink(file(), cat("a", file = std))
a

Adding the functionality to Reporter was easiest, other designs might be better.

Follow-up for #395.

Closes #419 (=contains it).

Comment thread R/reporter.R
warning("append ignored", call. = FALSE)
}

cat(..., file = self$out, sep = sep, fill = fill, labels = labels)
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.

Rather than having our own clone of cat(), I'd rather have something that's tweaked to make like a bit easier:

cat_line <- function(...) {
  cat(..., "\n", sep = "")
}

Not sure what to call the version that doesn't add a nl

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.

I'd like to tackle cat() refactoring in a separate PR, this is already big enough.

Caveat: Not all cat() calls have sep = "" in place, but the integration tests should catch this.

@hadley
Copy link
Copy Markdown
Member

hadley commented Mar 22, 2016

This looks like a great approach!

@krlmlr
Copy link
Copy Markdown
Member Author

krlmlr commented Mar 22, 2016

Thanks. From a design perspective it probably should be a mixin (can R6 do it?), or a separate class that is instantiated and used by each interested reporter.

@krlmlr
Copy link
Copy Markdown
Member Author

krlmlr commented Mar 22, 2016

Added NEWS.

@krlmlr
Copy link
Copy Markdown
Member Author

krlmlr commented Mar 22, 2016

Also added cat_tight(), cat_line() and cat_paragraph().

Comment thread R/reporter-tap.R
for (i in 1:self$n) {
if (!is.na(self$contexts[i])) {
cat("# Context", self$contexts[i], "\n")
self$cat_line("# Context ", self$contexts[i], " ")
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.

@dkesh: I'm reworking output for the TAP reporter. Does it require trailing whitespace, or would it work without them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace isn't required by TAP.

On Tue, Mar 22, 2016 at 6:41 PM Kirill Müller notifications@github.com
wrote:

In R/reporter-tap.R
#420 (comment):

   for (i in 1:self$n) {
     if (!is.na(self$contexts[i])) {
  •      cat("# Context", self$contexts[i], "\n")
    
  •      self$cat_line("# Context ", self$contexts[i], " ")
    

@dkesh https://github.com/dkesh: I'm reworking output for the TAP
reporter. Does it require trailing whitespace, or would it work without
them?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/hadley/testthat/pull/420/files/c1146de2c3ca0d721fb7e3cf3ce3ade9e2a9f81f#r57089060

@hadley hadley merged commit c1146de into r-lib:master Mar 23, 2016
@hadley
Copy link
Copy Markdown
Member

hadley commented Mar 23, 2016

Thanks - this is great 😄

@krlmlr krlmlr deleted the feature/error-in-message-redir branch March 24, 2016 10:44
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.

3 participants