Move the Ion value types to the types module #543
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
=======================================
Coverage 83.07% 83.07%
=======================================
Files 83 83
Lines 16126 16126
Branches 16126 16126
=======================================
Hits 13396 13396
Misses 1566 1566
Partials 1164 1164
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Looks mostly good to me. Minor cleanup asks below.
I do wonder if types::iterators having stuff that is germane to Element/ElementsIterator and Annotations iterators are in the right place. Generally iterator implementations should live near what they iterate over. So we either break out those things that are relevant to the type module or refactor it as a separate PR. Thoughts?
| use crate::types::IntAccess; | ||
| use crate::types::Str; | ||
| use crate::types::SymbolId; | ||
| use crate::types::{Blob, Clob}; | ||
| use crate::{ | ||
| raw_reader::BufferedRawReader, Decimal, Int, IonReader, IonResult, IonType, RawStreamItem, | ||
| RawSymbolToken, Timestamp, | ||
| raw_reader::BufferedRawReader, types::Decimal, Int, IonReader, IonResult, IonType, |
There was a problem hiding this comment.
I would consolidate these crate::types::{...} imports, especially the one that says types::Decimal.
| use crate::types::Decimal; | ||
| use crate::types::Timestamp; | ||
| use crate::types::{ContainerType, SymbolId}; |
| use crate::types::Symbol; | ||
| use crate::types::{Blob, Clob}; |
| // Re-export the Value variant types and traits so they can be accessed directly from this module. | ||
| pub use self::bytes::Bytes; | ||
| pub use crate::types::Bytes; | ||
| pub use crate::types::{Blob, Clob}; | ||
| pub use annotations::{Annotations, IntoAnnotations}; | ||
| pub use lob::{Blob, Clob}; | ||
|
|
||
| pub use list::List; | ||
| pub use r#struct::Struct; | ||
| pub use sequence::Sequence; | ||
| pub use sexp::SExp; | ||
| pub use crate::types::List; | ||
| pub use crate::types::SExp; | ||
| pub use crate::types::Sequence; | ||
| pub use crate::types::Struct; |
There was a problem hiding this comment.
I think I would at least put these together so they rustfmt together.
| use crate::types::Decimal; | ||
| use crate::types::UInt; | ||
| use crate::types::{Coefficient, Sign}; |
There was a problem hiding this comment.
Probably should make these all types::{...} or each on their own line.
| use crate::types::Decimal; | ||
| use crate::types::{FractionalSecondSetter, Timestamp}; |
There was a problem hiding this comment.
Consider consolidating these imports/making them consistent.
| use crate::types::UInt; | ||
| use crate::types::{Coefficient, Sign}; |
There was a problem hiding this comment.
Consider consolidating these imports/making them consistent.
| use crate::types::Decimal; | ||
| use crate::types::{Coefficient, Sign}; |
There was a problem hiding this comment.
Consider consolidating these imports/making them consistent.
src/types/iterators.rs
Outdated
| pub(crate) current: usize, | ||
| pub(crate) indexes: Option<&'a IndexVec>, | ||
| pub(crate) by_index: &'a Vec<(Symbol, Element)>, |
There was a problem hiding this comment.
See my top-level comment, this seems to indicate it is not in the right place.
Co-authored-by: Almann Goo <[email protected]>
|
@almann I have moved the Element and Symbol iterators back to the |
LGTM
Issue #, if available:
None.
Description of changes:
This moves the "types" to the
typesmodule, as per:It also makes it so that all of the types in the
typesmodule are exposed as part oftypesrather than as part of a submodule oftypes.Note that this is just fixing the internal organization of this code. How these types are exposed publicly is another matter that should also be fixed. In some cases, we are exporting the same type at multiple locations. (e.g.
ion_rs::Symbol,ion_rs::types::Symbol,ion_rs::element::Symbol) and that's just asking for confusion.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.