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

Implements IntoIterator for container Elements #499

Merged
merged 7 commits into from
Mar 29, 2023
Merged

Conversation

zslayton
Copy link
Contributor

This PR adds implementations of IntoIterator for each of the container Element types, allowing the expected looping syntax:

for element in &list {
    // ...
}

for element in &sexp {
    // ...
}

for (field_name, value) in &struct_ {
    // ...
}

It also moves the container types to their own modules for easier navigation.

Each commit is self-contained. Only the final commit (Implements IntoIterator...) contains any new logic. The rest are mechanical refactorings. I recommend reviewing them in order.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@desaikd desaikd left a comment

Choose a reason for hiding this comment

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

🚀

}
}

/// An in-memory representation of an Ion Struct
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add macro examples here as well?


#[test]
fn for_field_in_struct() {
// Simple example to exercise List's implementation of IntoIterator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Simple example to exercise List's implementation of IntoIterator
// Simple example to exercise Struct's implementation of IntoIterator

Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

🗺️ PR tour

Comment on lines +40 to +48
// Allows `for element in &list {...}` syntax
impl<'a> IntoIterator for &'a List {
type Item = &'a Element;
type IntoIter = ElementsIterator<'a>;

fn into_iter(self) -> Self::IntoIter {
self.elements()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This impl (and its unit test) is the only new code in this file; the rest was moved here from mod.rs.

use std::fmt::{Display, Formatter};

pub mod builders;
mod element_stream_reader;
mod iterators;
pub mod owned;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The owned module was emptied out in a prior PR but I forgot to remove it.

pub mod reader;
mod sequence;
mod sexp;
mod r#struct;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ struct is a Rust keyword and so needs to be escaped when used as a module name. The module itself is not public, however, so this clunkiness is not exposed to users. The Struct type is re-exported from mod.rs.

pub mod writer;

/// Behavior that is common to both [SExp] and [Struct].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The IonSequence trait was moved to the new sequence module.


/// An in-memory representation of an Ion list
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct List {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ The List type was moved to the new list module.

@@ -358,20 +53,6 @@ impl IonEq for Value {
}
}

impl IonEq for Vec<Element> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This impl was moved to the new sequence module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This module was emptied out in #491 but I forgot to delete the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ All of this was moved here from mod.

Comment on lines +40 to +48
// Allows `for element in &sexp {...}` syntax
impl<'a> IntoIterator for &'a SExp {
type Item = &'a Element;
type IntoIter = ElementsIterator<'a>;

fn into_iter(self) -> Self::IntoIter {
self.elements()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This impl (and its unit test) is the only new code in this module. Everything else was moved here from mod.

Comment on lines +164 to +172
// Allows `for (name, value) in &my_struct {...}` syntax
impl<'a> IntoIterator for &'a Struct {
type Item = (&'a Symbol, &'a Element);
type IntoIter = FieldIterator<'a>;

fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺️ This impl (and its unit test) is the only new code in this module. Everything else was moved here from mod.

@zslayton zslayton merged commit 9da0d76 into main Mar 29, 2023
@zslayton zslayton deleted the containers-into-iter branch March 29, 2023 21:18
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.

None yet

3 participants