Repository navigation
Add a new to_value method to Quantity. - #6127
Conversation
This returns the value of the quantity in the given unit.
mhvk
added
Affects-dev
|
Doc and remote test fails appear unrelated. |
|
Why is this marked |
mhvk
added
Affects-release
and removed
Affects-dev
|
I've never understood Sadly, the documentation failure was real, but it made me realize I should ensure this works for |
|
@mhvk - Basically both The other usecase for |
adrn
left a comment
adrn
left a comment
There was a problem hiding this comment.
This looks fine to me, though I'm somewhat neutral on the concept of the addition. On one hand, we now have two ways to effectively do the same thing with a difference of just a few characters. On the other hand, I do find myself typing .to(...).value quite often in my own code!
One open question I'm pondering is whether we want to take a stance and explicitly recommend using to_value() over to(...).value. If so, we should replace any instances of the latter in the documentation with the new method.
|
@adrn - |
|
@mhvk Hm, yea, I see 205 matches with |
|
@adrn - How about a post-merge PR to do this with a grep/replace? This is one place where perl rocks: http://www.perlmonks.org/?node_id=608701 |
|
👍 to do the replaces separately (btw I would do it with sed, but I believe perl can do this, too). |
|
Excellent news if this is faster ! |
|
OK sounds good. I've created an issue just so we don't forget. Tests are passing and doubly-approved, so merging now. |
|
Would the performance enhancement resulted from this deserve a |
|
In principle, yes, but I think that would at the minimum involve getting |
|
Running it on a CI is really not an option? I expect the general trends to show up there even on travis, and maybe circle or appveyor would work better? |
This returns the value of the quantity in the given unit.
@taldcroft: I ended up with
to_valueas I felt it was a bit more uniform (withto_string, etc.) and makes more sense without arguments (a form that needs to exist to make the property work, i.e.,q.to_value()is obvious butq.value_in()is not). I also ended up not using it forto, as especially the scalar case is slowed down a little too much by first doingvalue.item()and then recreating an array in_new_view.Fixes #6125