Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

JackKelly
Copy link
Contributor

Closes #61

@JackKelly JackKelly marked this pull request as ready for review October 3, 2024 16:37
@JackKelly
Copy link
Contributor Author

OK, this is ready for review 🙂

Once you're happy with this approach, I'll add derive(FromAbbrevStr) to the other product enums.

Copy link
Owner

@mpiannucci mpiannucci left a 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

@JackKelly
Copy link
Contributor Author

Great!

Just now, I added:

  • derive(FromAbbrevStr) for all the parameter enums.
  • a simple unit test

I think this is ready to merge into main now but please shout if you'd like any changes!

@JackKelly
Copy link
Contributor Author

JackKelly commented Oct 8, 2024

Actually, I'd like to add one more feature to this PR, if that's OK... I'd like a single method which takes a &str, checks through all the abbrev strings, and returns the appropriate enum variant.

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 from_str!

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!

@JackKelly
Copy link
Contributor Author

JackKelly commented Oct 8, 2024

So... the short answer is that I'm no longer planning to implement a method which takes an abbrev string and returns an enum variant.

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 enum ParameterEnum as shown above, with the following implementation:

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 hypergrib I'm actually going to keep the product abbreviations as strings (and not convert to gribberish types) and, in the future, if I do need to get the unit etc. for a given abbreviation then I might parse the GRIB tables recorded as .csv files in the gdal.

So this PR is ready for review & to be merged if it's OK!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement FromAbbrevStr procedural derive macro?
2 participants