Closed lewinfox closed 4 years ago
Thanks for looking into this. I think that's a very nice feature. Let's see what @edzer thinks.
Some comments on the implementation:
str(object, ...)
, with dots, so we need to respect that.NextMethod()
instead of str(unclass(object))
.print.units
does.mixed_units
.Good feedback, thanks. I'll put something together and see what you think 👍
On Tue, 3 Mar 2020, 02:52 Iñaki Ucar, notifications@github.com wrote:
Some comments on the implementation:
- The generic is defined as str(object, ...), with dots, so we need to respect that.
- Use NextMethod() instead of str(unclass(object)).
- I would add "Units: " to the beginning, as print.units does.
- A similar method should be implemented for mixed_units.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/r-quantities/units/issues/227?email_source=notifications&email_token=AGC6L2RH65IQ5PEPESUYUBDRFO22HA5CNFSM4K7QUZZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENPMB4Q#issuecomment-593412338, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGC6L2RM4SBID5SCTMJHX3TRFO22HANCNFSM4K7QUZZA .
@Enchufa2, I've made the suggested changes with the exception of replacing str(unclass(object))
with NextMethod()
.
The rationale here is NextMethod()
doesn't allow you to use capture.output()
, meaning you can't trim the output (although this SO post gives a workaround). The output of NextMethod()
is verbose so I think it would be good to limit it, but I haven't done a performance comparison of the two approaches. Is this something you'd like me to look at?
Below is a screenshot of the new functions at work. The only visual issues I can see are:
str.mixed_units()
is clipped in the RStudio viewer - the summary line is removed. Compare the terminal output with the environment pane in the screenshot below to see what I mean.Let me know what you think.
Thanks, comments inline:
@Enchufa2, I've made the suggested changes with the exception of replacing
str(unclass(object))
withNextMethod()
.The rationale here is
NextMethod()
doesn't allow you to usecapture.output()
, meaning you can't trim the output (although this SO post gives a workaround). The output ofNextMethod()
is verbose so I think it would be good to limit it, but I haven't done a performance comparison of the two approaches. Is this something you'd like me to look at?
The solution in the SO post is not really a workaround, but how capture.output
works: by default, it opens a textConnection
, then redirects the output using sink
. I believe we should use that mechanism here to be able to call NextMethod
, because units
should play well with errors
and quantities
, and avoiding NextMethod
makes everything else harder.
Below is a screenshot of the new functions at work. The only visual issues I can see are:
- The output of
str.mixed_units()
is clipped in the RStudio viewer - the summary line is removed. Compare the terminal output with the environment pane in the screenshot below to see what I mean.
What happens if you strip out the "List of x" line from the default?
- There is no whitespace between the colon and the start of the word "Units" in the list and data.frame views. Not sure why this is the case.
That's because str
by default inserts a whitespace (try, e.g., str(1:10)
). We should do the same.
Thanks, that's really useful. I hope to look at this over the weekend.
So I've modified things to use NextMethod()
as you suggested. I think we're nearly there, but I can't get a standalone mixed_units
object to display properly in the environment pane. See screenshot below - it looks great in the console but Rstudio is stil inserting List of n
in the environment pane. I've asked here and will see if they can shed any light as I'm stumped!
I assume it's something to do with this:
library(units)
mix <- units::mixed_units(1:5, c("s", "m", "cd", "kg", "lb"))
class(mix)
## "mixed_units"
inherits(mix, "list")
## FALSE
is.list(mix)
## TRUE
Current code is here.
Looks very good, thanks for all the work! You mean the "List of 5" on the right side? I think that's fine. It really is a list of 5, so no problem I'd say. I believe we are ready for a PR. :)
Hi, I was working with this package (which is great, by the way - thank you) and I wanted to make unit information visible in the RStudio environment pane rather than the default "object of class units".
Per the discussion here I've solved my issue by modifying
str.units()
in misc.R to incorporate some code from theprint.units()
method:Couple of screenshots of RStudio with the new function:
Is this something you'd be interested in including in the package? If so I can submit a PR.
I appreciate not everyone uses RStudio and some may have uses for
str()
that this overwrites, but I found it useful. Let me know what you think - if you think it's worth exploring I'm happy to tweak it if necessary.Thanks, Lewin