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

Add FromIterator implementations for Collection and CollectionWithId #291

Closed
wants to merge 1 commit into from

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Jul 16, 2019

Proposition to add the FromIterator to the custom collections we have in transit_model: Collection and CollectionWithId.

This would allow us to do replace a code like the following

let mut stop_areas = CollectionWithId::default();
for stop_area in reader.deserialize() {
    stop_areas.push(stop_area?)?;
}

into something which avoid to declare mutable fields and make good use of the Iterator.

let stop_areas: CollectionWithId<StopArea> = reader
  .deserialize()
  .collect();

{
iter.into_iter()
.fold(CollectionWithId::default(), |mut accumulator, object| {
accumulator.push(object);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one problem here. The function .push() returns a Result<> which means, this can fails. The current implementation here will just ignore the error totally (usually, there is compiler warning for an unused Result but it doesn't show anymore because of #![allow(unused_must_use)] above).

This means that the implementation will ignore duplicate (see last test below for an example). The trait FromIterator doesn't really provide a way to fail. And I couldn't find a TryFromIterator trait and a corresponding .try_collect() method (which I guess would have solved my problem here).

@woshilapin
Copy link
Contributor Author

Just some update about this Draft PR. The problem lies with CollectionWithId implementation, not with Collection. Indeed, in the case of CollectionWithId trying to collect 2 objects with the same identifier is a problem since they can't both live in the same CollectionWithId. I see a few options here.

1. Ignore duplicates

Collecting a duplicate identifier will simply ignore it, without raising an error.

+ the code doesn't panic
- no warning about what is happening and you lose some data without noticing it

2. Update duplicates

When a duplicate identifier is being collected, then the new one will be inserted in place of the old one.

+ the code doesn't panic
+ might seems logical to insert them in order so the last one wins
- no warning about what is happening and you lose some data without noticing it

3. Panic on duplicate

We see this kind of behavior sometimes in the standard library (for example, HashMap::reserve and HashMap::try_reserve). The idea is to use one or the other depending of what you are doing. But basically, using reserve is like using unwrap, the code may panic, whereas using try_reserve is more like using unwrap_or, you're sure the code won't panic...

Note: I couldn't find a TryFromIterator with a try_collect that would have solve the problem.

+ the code does show there is an error
+ no loss of data
- the code panic, it'd be better to return an error
- the corresponding `try_collect()` doesn't exists in the standard API, neither the [`TryFromIterator`]

4. Do not implement FromIterator for CollectionWithId

If there is too many problems with a choice about how to implement it, maybe the solution is to not implement it.

+ no problem
- no nice solution to collect an iterator into a `CollectionWithId`

@woshilapin
Copy link
Contributor Author

After some thoughts, no confortable solution is possible for CollectionWithId. So it will be removed.

As for the implementation of IntoIterator for Collection, it would be used nowhere.

I'm closing this PR. Maybe we can come back to it later if we have the need.

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

Successfully merging this pull request may close these issues.

1 participant