-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement FromAbbrevStr derive macro #62
base: main
Are you sure you want to change the base?
Conversation
OK, this is ready for review 🙂 Once you're happy with this approach, I'll add |
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 is awesome! All for it
Great! Just now, I added:
I think this is ready to merge into |
UPDATE: I'm no longer planning to add this feature. Please see the comment below! But I'll leave my notes about how we could implement this in the future if we wanted... I made a start implementing this feature like this: pub enum ParameterEnum {
// Oceanographic
WavesProduct(oceanographic::WavesProduct),
CurrentsProduct(oceanographic::CurrentsProduct),
SurfacePropertiesProduct(oceanographic::SurfacePropertiesProduct),
IceProduct(oceanographic::IceProduct),
// Land surface
VegetationProduct(land_surface::VegetationProduct),
// Meteorological
TemperatureProduct(meteorological::TemperatureProduct),
MoistureProduct(meteorological::MoistureProduct),
MomentumProduct(meteorological::MomentumProduct),
CloudProduct(meteorological::CloudProduct),
MassProduct(meteorological::MassProduct),
RadarProduct(meteorological::RadarProduct),
ForecastRadarImagery(meteorological::ForecastRadarImagery),
Electromagnetics(meteorological::Electromagnetics),
PhysicalAtmosphericProperties(meteorological::PhysicalAtmosphericProperties),
// MRMS
MRMSLightningProduct(mrms::MRMSLightningProduct),
MRMSConvectionProduct(mrms::MRMSConvectionProduct),
MRMSPrecipitationProduct(mrms::MRMSPrecipitationProduct),
MRMSCompositeReflectivityProduct(mrms::MRMSCompositeReflectivityProduct),
MRMSMergedReflectivityProduct(mrms::MRMSMergedReflectivityProduct),
}
impl FromStr for ParameterEnum {
type Err = ParseAbbrevError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
if let Ok(p) = oceanographic::WavesProduct::from_str(s) {
return Ok(Self::WavesProduct(p));
} else if let Ok(p) = oceanographic::CurrentsProduct::from_str(s) {
return Ok(Self::CurrentsProduct(p));
} // TODO: continue these `else if let` blocks...
Err(ParseAbbrevError(format!("Could not parse {s}")))
}
} But this will result in a huge (and ugly) implementation of I've looked into whether we can make a macro which could register all the parameter enums. But there doesn't seem to be a way to do that. So I'm probably going to go with an approach like this: pub fn parameter_enum_from_abbrev(
abbrev: &str,
) -> Result<impl std::fmt::Display + std::convert::From<u8> + std::str::FromStr, ParseAbbrevError> {
let from_str_methods = [oceanographic::WavesProduct::from_str];
for from_str in from_str_methods {
if let Ok(variant) = from_str(abbrev) {
return Ok(variant);
}
}
Err(ParseAbbrevError(format!("Could not parse {abbrev}")))
} Pls let me know if you have any preferences! |
So... the short answer is that I'm no longer planning to implement a method which takes an But, just for future reference, if we did want to implement such a method then I think a reasonable approach might be to define a impl FromStr for ParameterEnum {
type Err = ParseAbbrevError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
oceanographic::WavesProduct::from_str(s)
.map(|p| Self::WavesProduct(p))
.or_else(|_| {
oceanographic::CurrentsProduct::from_str(s)
.map(|p| Self::CurrentsProduct(p))
})
.or_else(|_| {
oceanographic::SurfacePropertiesProduct::from_str(s)
.map(|p| Self::SurfacePropertiesProduct(p))
})
// etc.
}
} Which could perhaps be automated using a proc macro. But, after thinking more about this, I think that in the So this PR is ready for review & to be merged if it's OK! |
Closes #61