Improvement requests

[gergelytraveltime]

1. A little bit weird error handling in main. Could it be improved maybe?
2. It would be good to avoid mutable variables
3. No mapping on error necessary for deserializing DecimalLon? Are there any possible invalid Longitudes?

   
   impl<'de> Deserialize<'de> for DecimalLat {
   fn deserialize<D>(deserializer: D) -> Result<DecimalLat, D::Error>
   where
       D: Deserializer<'de>,
   {
       DecimalLat::new(f64::deserialize(deserializer)?).map_err(|e| serde::de::Error::custom(e))
   }
   }

impl<'de> Deserialize<'de> for DecimalLon { fn deserialize(deserializer: D) -> Result<DecimalLon, D::Error> where D: Deserializer<'de>, { Ok(DecimalLon::new(f64::deserialize(deserializer)?)) } }

4. Do you have a "one-off" error here? If I test your code I get this error for a polygon with 3 vertices.
```rust
        if vertices.len() < 4 {
            return Err(serde::de::Error::custom("Polygon must have at least 3 vertices"));
        }

5. In data_structures.rs: aren't the last two cases testing the same thing? Are you checking the error message? If you were, I think you'd see that both detect the same error. This may be related to the previous issue I highlighted.

[fildaw]

Thank you for the code review.

1. My intention with .expect() calls was to panic when an error which cannot be recovered from (without assuming some default behaviour) occurred.
2. I would try to remove mutable variables.
3. For DecimalLon I had a similar error mapping as for DecimalLat, but because I allowed unnormalized longitudes, I removed it. Nevertheless, maybe I should implement back the limit, but in range [-360,360].
4. The error message is ambiguous, because I meant that in fact the polygon must have at least 3 vertices, but because I force the polygon to start and end with the same coordinate (to close it) it effectively must have 4 coordinates.
5. That's a typo which I would certainly fix. I had some problems to dig into error messages from this state, and that's why I'm checking only 'kind' of the errors, I would try to improve it to check error messages.

[gergelytraveltime]

CC: @PKJonas, @danielnaumau

[PKJonas]

Related to 2., this part seems a bit odd:

    #[serde(rename="coordinates", skip_serializing)]
    pub polygons: Vec<Polygon>,
    #[serde(skip_deserializing)]
    pub matched_locations: Vec<Location>,

I'm not familiar with Rust but skip_serializing and skip_deserializing feel like a code smell, like you're making a single type do the work of two different types: a Region like you have, and a MatchResult (or something) which would hold the region name and matched locations.

[gergelytraveltime]

A bit more on point 2.:

It would be good to avoid mutable variables/data in your code, like with regions. It'd be better if you created a new type.

[gergelytraveltime]

We are good with your proposals, please try avoid any ambiguity for 4. and 5., make error messages crystal-clear (also for the reviewer/read of your code) and try to ensure to really test that behavior in unit tests.

[PKJonas]

Re 1.: when using .expect this way, what would be the user experience if the user had multiple errors? Say if both region and location files had errors in them, or one had an error and one had a typo in the path.

The answer would suggest why collecting errors is nice even if you can't recover from any of them.