jcgay/maven-profiler

Report: Mixed units

Closed this issue · 4 comments

Currently, units in the report are mixed (e.g. the report contains ms and s). This makes it difficult to reuse the values provided in the reports.

image

It would be nice to be able to specify a unit for the report which is then used consistently, e.g. ms

jcgay commented

You're right and it should be the default for JSON output.
I'm just wondering what do you mean by "reuse the result" of the HTML output ? Processing the JSON is not easier in your use case ?

Thanks for the feedback.

You are right, JSON might be easier, thank you for the pointer.

For JSON, would it suffice to change https://github.com/jcgay/maven-profiler/blob/master/src/main/java/fr/jcgay/maven/profiler/ProfilerEventSpy.java#L294 and https://github.com/jcgay/maven-profiler/blob/master/src/main/java/fr/jcgay/maven/profiler/ProfilerEventSpy.java#L299
or do I miss something?

To implement the idea cleanly, would that require to introduce a property for the "target unit"?

jcgay commented

Indeed it should do the job by modifying the lines you have spotted. It would also required to change ProfilerEventSpy.java#L312

For the JSON I think the report can always use milliseconds and it may be useful to separate the value from its unit to be easier to parse.
For the HTML, a property could be added if you feel the need to. From my point of view it's easier to read the way it is now but it can be make configurable.

I will be able to work on these later this week but if you are willing to contribute the changes, I'll be glad to merge them 😄

jcgay commented

Finally took the time to release it, thanks again @BenjaminHerbert !