New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add tests for explicit/stable tag->u16 conversion #1150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion functions can be automatically derived through macros, we already have that in place:
lurk-rs/lurk-macros/src/lib.rs
Lines 476 to 531 in 3e86743
/// This macro derives an impl of TryFrom<foo> for an enum type T with `#[repr(foo)]`. | |
/// | |
/// # Example | |
/// ``` | |
/// use lurk_macros::TryFromRepr; | |
/// | |
/// #[derive(TryFromRepr)] | |
/// #[repr(u8)] | |
/// enum Foo { | |
/// Bar = 0, | |
/// Baz | |
/// } | |
/// ``` | |
/// | |
/// This will derive the natural impl that compares the input representation type to | |
/// the automatic conversions of each variant into that representation type. | |
#[proc_macro_derive(TryFromRepr)] | |
pub fn derive_try_from_repr(input: TokenStream) -> TokenStream { | |
let ast = parse_macro_input!(input as DeriveInput); | |
let res_ty = get_type_from_attrs(&ast.attrs, "repr"); | |
let name = &ast.ident; | |
let variants = match ast.data { | |
Data::Enum(ref variants) => variants | |
.variants | |
.iter() | |
.map(|v| &v.ident) | |
.collect::<Vec<_>>(), | |
Data::Struct(_) | Data::Union(_) => { | |
panic!("#[derive(TryFromRepr)] is only defined for enums") | |
} | |
}; | |
match res_ty { | |
Err(e) => { | |
// If no explicit repr were given for us, we can't pursue | |
panic!("TryFromRepr macro requires a repr parameter, which couldn't be parsed: {e:?}"); | |
} | |
Ok(ty) => { | |
let match_arms = try_from_match_arms(name, &variants, &ty); | |
let name_str = name.to_string(); | |
quote! { | |
impl std::convert::TryFrom<#ty> for #name { | |
type Error = anyhow::Error; | |
fn try_from(v: #ty) -> Result<Self, <Self as TryFrom<#ty>>::Error> { | |
match v { | |
#match_arms | |
_ => Err(anyhow::anyhow!("invalid variant for enum {}", #name_str)), | |
} | |
} | |
} | |
} | |
} | |
} | |
.into() | |
} |
@huitseeker the idea is to have hardcoded values that have to be changed by hand if someone changes the order of enum variants. See #1141, which was created after #1140 implemented a fix for the order swap introduced in #1133 |
In other words, we want to counter-balance the automation we already have with (healthy) friction that #1133 didn't have (i.e. the need to explicitly change the tests if the tag enums are changed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me
This PR adds tests that enforce stability for
Tag
->u16
conversions.Closes #1141 with a solution that's simpler than the manual re-implementation of conversion functions.