Closed shahn closed 8 months ago
Attention: 29 lines
in your changes are missing coverage. Please review.
Comparison is base (
dd82a9c
) 57.32% compared to head (9763ccd
) 57.24%.
Files | Patch % | Lines |
---|---|---|
src/varint/decode_unsigned.rs | 68.91% | 23 Missing :warning: |
src/config.rs | 0.00% | 6 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Sorry for the late response, I was going to think about this over the weekend and then Factorio happened and now it's a month later.
A couple of things we've been thinking about:
derive
attributes we want on the enums, PartialEq, Eq
is maybe not needed, Clone
or Debug
might be? We're not sureconst fn
in traits. We were thinking of adding #[inline(always)]
to the implementation functions but I don't want to do that before we actually benchmark that it is faster/slower, and I think that's outside of the scope of this PR.Other than that it looks good, thanks for the contribution!
Thanks for the merge, I think PartialEq and Eq are needed to ensure you can actually do something with the config values. Debug could be nice, but is not required to do something useful. Or perhaps I misunderstood your question?
You understood my question correctly.
I think PartialEq and Eq are needed to ensure you can actually do something with the config values
You can still match on the variants. This is what we do in bincode currently, the following code will still work without PartialEq, Eq
:
Ok(match D::C::ENDIAN {
Endianness::Little => u16::from_le_bytes(bytes),
Endianness::Big => u16::from_be_bytes(bytes),
})
But this will not:
if D::C::ENDIAN == Endianness::Little {
panic!("Only big endian supported");
}
But I'm also not sure if there's a downside to adding this
This was inspired by suggestions provided in #598, thanks a lot to @VictorKoenders.
Fixes #598.