hove-io / transit_model

Managing transit data with Rust
GNU Affero General Public License v3.0
52 stars 28 forks source link

better object duplicate error #708

Open antoine-de opened 3 years ago

antoine-de commented 3 years ago

Hi, When using gtfs2netexfr we have some errors like "identifier 8593027 already exists" (as the last log of a netex generation). To help the diagnostic, it would be helpful to know the collection having duplicates and maybe explain at a meta level (in the main ?) that the problem blocks the netex generation.

datanel commented 3 years ago

I agree this would be helpful

woshilapin commented 3 years ago

This message comes directly from https://github.com/CanalTP/typed_index_collection/blob/v1.1.0/src/error.rs#L7. The type of object is not known inside this generic library (we know the struct but can't really display anything). I see a few options on how to handle this problem (there might be more).

Fix in typed_index_collection

Doing this probably means to add an additional constraint on T like T: Display or T: Debug in order to be able to display the struct of the type of object.

I'm not really in favor of that solution because:

Introduce a Trait

To fix the previous solution, we could introduce a specific trait typed_index_collection::ObjectType for example. It's still adding a constraint to T but at least, we do not depend on implementation of Debug or Display that might not be to our taste.

Handle errors in transit_model

Most likely, we're gonna be able to create better error in transit_model. With thiserror for example, we could do something like this.

enum ObjectType {
    Network,
    Line,
    Etc,
} // Implement Display on that `enum`

#[derive(Error)]
enum TMError {
    #[error("identifier {1} of type {0} already exists")]
    IdentifierAlreadyExists(ObjectType, String, #[source] typed_index_collection::Error)
    #[error("other error")]
    OtherError,
}

Since we have plans to migrate transit_model from failure to a more modern framework (thiserror has been discussed for example), maybe this issue can be adressed once the migration happened? Note that we plan to do it but we didn't scheduled it yet and have no idea when it could happen.

antoine-de commented 3 years ago

will be closed by https://github.com/CanalTP/typed_index_collection/pull/11 no?

woshilapin commented 3 years ago

I don't think so since this piece of code is not going to output the type of object. However, we might be a step closer to achieve that.