Relax filesizeformat filter requirements

[GuillaumeGomez]

Fixes #215.

Funnily enough, it also allowed to remove one dependency, so yeay! \o/

[GuillaumeGomez]

Finally fixed Ci \o/

[Kijewski]

Suggested change

	buf.write(format_args!(
	"rinja::filters::HtmlSafeOutput(rinja::filters::{name}(",
	"rinja::filters::HtmlSafeOutput(rinja::filters::{name}("
	));
	self._visit_args(ctx, buf, args)?;
	buf.write(")?)");
	buf.write(format_args!(
	"rinja::filters::HtmlSafeOutput(\
	rinja::filters::{name}(\
	rinja::helpers::get_primitive_value(&("
	));
	self._visit_args(ctx, buf, args)?;
	buf.write(
	"\
	)) as rinja::helpers::core::primitive::f32\
	)?\
	)"
	);

So filesizeformat() does only need to accept f32.

[GuillaumeGomez]

I'm a bit shared about this. With the current approach, if users have something like a newtype, they can just implement the ToF32 trait on it to make it work. With your suggestion, they'd need to make a different kind of conversion before being able to call the filter. It sounds a bit less practical.

[Kijewski]

I would rather make rinja::helpers::PrimitiveType public, so a user could implement something like that

impl PrimitiveType for DiskSpaceRequirements {
    type Value = u64;

    #[inline]
    fn get(&self) -> u64 {
        self.unpacked_size
    }
}

Otherwise, if you want to add an explicit trait, I would rename it to something like AsFilesize.

[GuillaumeGomez]

Good idea! Let's go with PrimitiveType as public trait. :)

[Kijewski]

I would explicitly use NbAndDecimal(u32): usize would be too small on 16bit systems, and on x64 sytems, working with u64 is a lot slower than u32.

[GuillaumeGomez]

I wondered about that but I guess you're right.

[Kijewski]

I wonder if this whole if-else-if block could be implemented as a loop: for (prefix, limit) in [("", 1e3), ("k", 1e6), ..] {?

[GuillaumeGomez]

It could, but the advantage of this approach is that we don't need to format the string of the units. :)

[Kijewski]

I guess we could use [" B", " kB", ..] including the space and 'B', and have fewer write calls.

[GuillaumeGomez]

Now I'm not sure to follow what you mean. I have in mind:

for (unit, div) in [(" KB", 1e6), ...] {
    if self.0 < div {
        f.write_fmt(format_args!("{}{unit}", NbAndDecimal::new(self.0)))
    }
}

That's still one extra argument formatting.

[Kijewski]

write!(f, "{}", NbAndDecimal::new(self.0))?;
f.write_str(unit)?;

The extra call is there in the current implementation, too, it is just hidden in the unrolled code.

[GuillaumeGomez]

Ok I see. Let's go with it then. Hopefully, loop will be unrolled at compile-time but it's not a hot path so should be fine.

[GuillaumeGomez]

Applied suggestions and fixed CI. :D

[Kijewski]

I made some changes to the implementation, and pushed it to your PR.

To test if my idea how to optimize NbAndDecimal could work, I kinda had to implement it. I hope that's OK! :-/

[GuillaumeGomez]

Yup it's fine. I was going through the code and I discovered that I badly handled the case where the decimal ends with '0' and when the number is still too big (where we should not show decimals). Great improvements! :D

[Kijewski]

I think removing a feature is a breaking change. Maybe it would be better to keep it as a noop for now?

[GuillaumeGomez]

We can make the next release a major one. Problem solved like that. :)

[Kijewski]

Works for me! :D

[GuillaumeGomez]

Thanks a lot!