Codecov Report

Merging #255 (432d476) into master (866563e) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #255   +/-   ##
=======================================
  Coverage   94.39%   94.39%           
=======================================
  Files          17       18    +1     
  Lines         856      857    +1     
=======================================
+ Hits          808      809    +1     
  Misses         48       48

Impacted Files	Coverage Δ
R/helpers.R	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 866563e...432d476. Read the comment docs.

edzer commented 3 years ago

I can see the point of keep_units(), but find adapt_units a bit namespace clobbering, as it doubles set_units essentially wrapping its mode argument. Can't we make set_units more clever and have it choose mode automatically? The problem is really here:

> a = set_units(1, m)
> set_units(a, set_units(1, km))
Error: could not find function "set_units"
Did you try to supply a value in a context where a bare expression was expected?
> set_units(set_units(1, m), set_units(1, km), mode = "standard")
0.001 [km]

the error is not helpful, and always when I use set_units in code, I use standard mode, and I have to look up its documentation because I forgot.

Enchufa2 commented 3 years ago

Ok, I'll rebase this PR and work on making set_units more clever when we resolve the others.

Enchufa2 commented 3 years ago

I've been thinking about making set_units more clever, and it may break current functionality. Suppose we want this to work:

x <- set_units(1000, m)
set_units(x, make_units(km))

The problem is that we would need to actually evaluate the value argument to check whether it is a units or a symbolic_units object, to switch to standard evaluation in such a case. But then NSE evaluation is broken depending on the user's environment. E.g.,

km <- set_units(1, l)
set_units(x, km)

would fail, because we cannot convert meters to liters.

To try to avoid such breakage, we could 1) try symbolic evaluation, 2) try standard one if the symbolic fails, 3) throw the first error if the second fails too. But then the method gets very inefficient (more try expressions in the execution path) and still there are many cases in which both methods would succeed.

So I think that set_units should stay the same, even if we drop adapt_units from this PR. IMHO, set_units never should have allowed NSE for units in the first place. But now we have to deal with it. :)

r-quantities / units

New helper keep_units #255

Codecov Report