Skip to content

Implement FromAbbrevStr derive macro#62

Open
JackKelly wants to merge 6 commits into
mpiannucci:mainfrom
JackKelly:jack/FromAbbrevStr-macro
Open

Implement FromAbbrevStr derive macro#62
JackKelly wants to merge 6 commits into
mpiannucci:mainfrom
JackKelly:jack/FromAbbrevStr-macro

Conversation

@JackKelly

Copy link
Copy Markdown
Contributor

Closes #61

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

Copy link
Copy Markdown
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.

@mpiannucci mpiannucci left a comment

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

JackKelly commented Oct 8, 2024

Copy link
Copy Markdown
Contributor Author

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

JackKelly commented Oct 8, 2024

Copy link
Copy Markdown
Contributor Author

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