bencheeorg / benchee

Easy and extensible benchmarking in Elixir providing you with lots of statistics!
MIT License
1.41k stars 66 forks source link

Implement human friendlier formatting for `Benchee.Conversion.Duration` #373

Closed drobnyd closed 9 months ago

drobnyd commented 1 year ago

As per issue https://github.com/bencheeorg/benchee/issues/332, it would be nice to format Duration in a more verbose fashion.

Another option would be to generalize the place_values/1 function and put it into the Scale module (and extend extended the behaviour with units/0 function returning all units for the implementor), and similarly implement format_verbose/1 as part of the Format module. However, I'm not sure if it's so useful to have this functionality for Count and Memory modules too but I'd like to hear your opinions!

eksperimental commented 1 year ago

ExUnit does not use a space between the number and the unit, therefore: "1 h 1 min 1 ms 1 μs"

would look like: "1h 1min 1ms 1μs"

drobnyd commented 1 year ago

ExUnit does not use a space between the number and the unit, therefore: "1 h 1 min 1 ms 1 μs"

would look like: "1h 1min 1ms 1μs"

Wouldn't omitting the space make the verbose format inconsistent with Format.format/1? Meaning that Benchee.Conversion.Duration.format(42) would return "42 ns" while Benchee.Conversion.Duration.format_verbose(42) would give us "42ns"?

Alternatively, we could change the default separator for Duration to align the two versions not to print the space but that would result in inconsistency with other units that are all using space as a separator.

While I agree that matching ExUnit's output makes sense, I think changing format of all durations is probably out of scope for #332 as it was not discussed and I'm not sure there is an agreement on removing the spaces. Also, it would probably be a slightly bigger change that would deserve its own PR. But if it's something that has wider support, I'm happy to work on it next.

What do you think @eksperimental?

eksperimental commented 1 year ago

I think they can be left as is, and create a new issue stating the the format should ideally align with the ExUnit's one. Thank you and sorry about the late response.

PragTob commented 9 months ago

Heyo,

sorry for not being around here forever. Your contribution is appreciated a lot and I'm very sorry for the bad contribution experience, lots of personal circumstances and more. I'll try to be better about all that going forward again.

I'll try to merge this and ship it as part of benchee 1.3.0.

IMG20230429163945

PragTob commented 9 months ago

Attempting to adjust and integrate this in #419 (as I have let this lie around for so long I see this as my responsibility). Thanks agin for all the work!

PragTob commented 9 months ago

merged as #419

eksperimental commented 8 months ago

Awesome. thank you @PragTob . Send a pat to your bunny on my behalf.