-
Notifications
You must be signed in to change notification settings - Fork 33
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
Module cleanup, adds Element::expect_*
, removes IntAccess
#595
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
==========================================
- Coverage 82.84% 82.45% -0.39%
==========================================
Files 111 111
Lines 19302 19399 +97
Branches 19302 19399 +97
==========================================
+ Hits 15990 15996 +6
- Misses 1770 1856 +86
- Partials 1542 1547 +5
☔ View full report in Codecov by Sentry. |
use ion_rs::RawStreamItem; | ||
use ion_rs::{BlockingRawBinaryReader, IonReader, IonType, RawReader, StreamItem, UserReader}; | ||
use ion_rs::{ | ||
BlockingRawBinaryReader, IonReader, IonResult, IonType, RawBinaryReader, RawReader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that we're exporting "raw" readers at the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really thought through how I'd like to expose all of the types that are behind experimental feature flags yet. Open to suggestions.
pub mod element; | ||
pub mod result; | ||
pub mod types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, just to make sure I'm understanding correctly, we're exporting everything here and at the root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're exporting everything here
There are some types that I don't think are commonly used enough (at least by name) to merit a spot in the crate root (OwnedElementIterator
, FractionalSecondSetter
, etc).
That said, there aren't that many of them and they're pretty distinctively named, so I could also be convinced not to expose any modules at all.
@@ -224,6 +214,8 @@ pub use { | |||
blocking_reader::{BlockingRawBinaryReader, BlockingRawReader, BlockingRawTextReader}, | |||
ion_reader::IonReader, | |||
raw_reader::{BufferedRawReader, RawReader, RawStreamItem}, | |||
raw_symbol_token::RawSymbolToken, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious. Do you think it would be helpful, neutral, or unhelpful for everything in the experimental-reader
feature to be exported under ion_rs::reader
as a way of making the namespaces somewhat match the features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends -- if we're leaning towards a pretty flat namespace, I don't think it would help that much. If we think we'll ultimately have visible modules, it might be somewhat helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we're leaning towards a flat namespace. I do think that the most common things should probably be at the front and center.
That being said, I think figuring this out would be a blocking issue for stabilizing the reader feature, but not for the v1.0 release.
@@ -15,7 +15,7 @@ use std::ops::Div; | |||
|
|||
/// Indicates the most precise time unit that has been specified in the accompanying [Timestamp]. | |||
#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Default)] | |||
pub enum Precision { | |||
pub enum TimestampPrecision { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR:
types
andelement
modules visible at the root level.Precision
toTimestampPrecision
since it is now visible at the root level.IntAccess
trait, which was introduced when we had theborrowed
implementation ofElement
. Theas_i64()
method is now natively available on bothElement
andInt
.TryFrom
/From
implementations forInt
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.