search

svartalf / rust-battery

Rust crate providing cross-platform information about the notebook batteries.
https://crates.io/crates/battery
Apache License 2.0
354 stars 40 forks source link

Consider removing uom #55

Open cjbassi opened 4 years ago

cjbassi commented 4 years ago

First of all I just want to say I really appreciate this crate!

The issues I've had with uom regarding this crate are:

  1. It was difficult to understand how to work with the uom return type when first using this crate
  2. For some reason uom needs to recompile every time I compile my program which takes time and is annoying
  3. I don't really feel like uom provides any value as a return type. Personally, I would prefer just a simple f32/f64 or you could even alias it to a custom type.

Let me know what you think.

svartalf commented 4 years ago

Hi, @cjbassi! Thank you, I really appreciate that :)

Regarding your questions:

  1. fwiw, battery docs has few extra examples on the types usage: https://docs.rs/battery/0.7.5/battery/units/index.html It also would be beneficial for everyone to contribute some examples to the uom.
  2. Last time I checked, uom spends a lot of time on macro expanding step, so there is nice opportunity for sending PR with compilation performance improvements; potentially breaking it into the smaller macros should reduce the compilation time. Also I'm not sure if @iliekturtles has plans to do that, but upcoming const generics probably will remove the typenum dependency, helping with the question of matter.
  3. Dimensional units not only cover some random theoretical logic bugs, but already helped to fix couple issues in this crate too. I'm strongly against returning back to the untyped primitive types, such as f32 / f64, there definitely should be some unit types crate used, and uom is best choice available.
iliekturtles commented 4 years ago

I'm not sure why uom would be recompiling every time your program is recompiled. That seems more like a cargo issue. If you can provide logs feel free to open an uom issue and I can help investigate.

In regards to compile time, last I investigated macro expansion wasn't the problem. Privacy checking (https://github.com/iliekturtles/uom/issues/52) was where all the time was spent. There is an upstream issue (https://github.com/rust-lang/rust/issues/50614) that is still open. I also have a branch to work on a uom 2.0 to speed up compile times among other things.

The typenum dependency will definitely be dropped as soon as const generics are ready. I haven't done any experimentation with what is currently available in nightly so I don't know how soon that will be.

cjbassi commented 4 years ago

Oh dang, I completely missed that section of the docs :) Well that makes more sense now, and I can see the benefits of it. I was also only using it for the percent.

In regards to the compilation issue, it seems that the issue is that typenum needs to recompile every time which requires uom to recompile too. But if typenum is getting dropped eventually anyway, maybe we can just ignore it for now.

iliekturtles commented 4 years ago

Even typenum shouldn't be re-building every time. I can't reproduce locally. If you can provide output from the following command that may help diagnose the issue.

cargo build --verbose ; cargo build --verbose