Clean up public API for v1.0.0 release#1024
Conversation
Gate items behind feature flags, remove vestigial types, fix un-nameable types in public signatures, and clean up re-exports. - Gate `AnyEncoding`, `ElementReader`, `ElementWriter`, `SymbolId`, `Catalog`, `EmptyCatalog`, `MapCatalog`, `SharedSymbolTable` behind `experimental-reader-writer` - Gate `HasRange`, `HasSpan`, `Span` behind `experimental-tooling-apis` - Remove `Format` enum and `IonResultIterExt` trait - Restrict `OwnedSequenceIterator` and `WriteConfig<*_1_1>::new()` visibility to `pub(crate)` - Re-export `SourceLocation` and `IonInput` at crate root - Move `Coefficient` and `Sign` to `ion_rs::decimal::*`, make `coefficient` submodule `pub(crate)` - Remove trait bounds from `ConversionOperationError` struct definition - Change `dep:digest`, `dep:sha2` to prevent implicit feature flags - Document public fields on `List`, `SExp`, `Blob`, `Clob` - Document `IonInput` as sealed - Define local `Format` enum in test harness (removed from public API)
These expose `num_traits` as a public dependency and lock in premature arithmetic semantics. Replace `Zero::is_zero()` with inherent methods. Arithmetic can be re-added in a minor release once the design is resolved (amazon-ion#970, amazon-ion#286). Verified: neither `ion-cli` nor `ion-schema-rust` uses any of these operations.
Chrono is an implementation detail (amazon-ion#838) that should not couple ion-rs's SemVer to chrono's. All `From`/`TryInto` impls between `Timestamp` and chrono types now require the `experimental-chrono` feature flag. Internal code uses `pub(crate)` helper methods instead of the trait impls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
==========================================
- Coverage 79.16% 79.06% -0.11%
==========================================
Files 140 140
Lines 34932 34809 -123
Branches 34932 34809 -123
==========================================
- Hits 27654 27520 -134
- Misses 5187 5209 +22
+ Partials 2091 2080 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
almann
left a comment
There was a problem hiding this comment.
Overall looks good--though I see some unused imports that might need conditional compilation to make them not complain.
| #[test] | ||
| fn demonstrate_try_filter_and_try_map() -> IonResult<()> { | ||
| let mut reader = Reader::new(AnyEncoding, r#"1 2 3 4 5 6 7 8 9 10"#)?; | ||
| let evens: IonResult<Vec<i64>> = reader | ||
| .try_filter(|element| Ok(element.ion_type() == IonType::Int)) | ||
| .try_map(|element| element.read()?.clone().expect_i64()) | ||
| .try_filter(|i| Ok(i % 2 == 0)) | ||
| .collect(); | ||
| drop(reader); | ||
| assert_eq!(vec![2, 4, 6, 8, 10], evens?); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Are these tests redundant and that is why they are removed?
There was a problem hiding this comment.
They were demonstrating the IonResultIterExt trait, which is being removed.
| /// Extension methods for iterators that return an `IonResult`. | ||
| /// | ||
| /// These methods are analogous to methods that already exist on `Iterator`, | ||
| /// but automatically handle situations where the input `IonResult` is an `Err` | ||
| /// sparing the user from writing more boilerplate. | ||
| // This code is not used within the library but is intentionally available to tests and applications. | ||
| pub trait IonResultIterExt<Item>: Iterator<Item = IonResult<Item>> { |
There was a problem hiding this comment.
Are we at all concerned about breaking any downstream with this?
There was a problem hiding this comment.
The trait has a pretty distinctive name, so I checked GitHub and Amazon internal code, and no one names or uses this trait.
If someone is using it in private code, they can either let us know (and we can re-add it) or recreate it themselves. If there is any breakage, it will be at compile time and easy enough to fix (unlike other languages where this sort of change might cause a runtime failure).
9074c1a to
b780cbf
Compare
Reduces the public API surface in preparation for v1.0.0. The goal is a minimal stable surface that we can grow from.
Items can always be added in minor releases but cannot be removed after 1.0.
This PR contains three commits:
79484b5— Gate, remove, or reorganize public API itemsf9858cb— Remove arithmetic trait impls from numeric types8bf799c— Gate chrono conversions behindexperimental-chronoIf you're interested, the full output of
cargo public-apiis available here, but what follows is a summary of the changes and the reasoning for each.Gate reader/writer-only types behind
experimental-reader-writer(79484b5)Several types were unconditionally public despite being useless without the streaming Reader/Writer. Consistent with the approach established in #576:
AnyEncoding— only meaningful when constructing aReaderElementReader,ElementWriter— only have implementations whenthe Reader/Writer is available
SymbolId— no stable API method accepts or returns itCatalog,EmptyCatalog,MapCatalog— only consumed byReaderSharedSymbolTable— only useful with catalogsGate tooling types behind
experimental-tooling-apis(79484b5)HasRange,HasSpan, andSpanexpose encoding-level byte details.Every implementor is already behind this feature flag, and users who need position info have
SourceLocationin the stable API (#810).Remove vestigial items (79484b5)
Formatenum — discussed in Consider MovingTextKindandFormatTypes #246 but never wired up; zero usagesIonResultIterExt— no known internal or external usersRestrict visibility of implementation details (79484b5)
OwnedSequenceIterator— users receive it viaIntoIterator, never need to name itWriteConfig<*Encoding_1_1>::new()— Ion 1.1 types not accessible without their feature flagFix un-nameable types in public signatures (79484b5)
SourceLocation— re-exported ation_rs::SourceLocation(It seemsPositionis not publicly nameable #809)IonInput— re-exported ation_rs::IonInput; enables generic functions over Ion input sources. Documented as sealed for now. (Long term intention is Give users a way to implementIonInputfor their own types #783, so making it public but sealed is fine for now. )Clean up decimal module exports (79484b5)
CoefficientandSignmoved toion_rs::decimal::Coefficientand
ion_rs::decimal::Signcoefficientsubmodule nowpub(crate)Remove leaked trait bounds from
ConversionOperationError(79484b5)Internal traits (
ValueTypeExpectation,TypeExpectation) removedfrom the struct definition; moved to impl blocks only.
Prevent implicit feature flags (79484b5)
digestandsha2changed todep:digest,dep:sha2so they canonly be enabled via
experimental-ion-hash.Remove arithmetic traits from
Int,UInt,Decimal(f9858cb)Add,Sub,Neg, andnum_traits::Zerowere unused outside oftests and exposed
num_traitsas a public dependency. Issues #970and #286 ask for arithmetic but have no design resolution —
committing now would lock in a premature decision. Inherent
is_zero()methods replaceZero::is_zero().Gate chrono behind
experimental-chrono(8bf799c)Chrono is an implementation detail (#838) that coupled ion-rs's
SemVer to chrono's. All
From/TryIntoimpls betweenTimestampand chrono types now require the feature flag. Internal code uses
pub(crate)helper methods.Potential correctness concern: The
From<NaiveDateTime>conversionis semantically suspect — Ion timestamps with unknown offset still
represent UTC instants, but
NaiveDateTimecarries no UTC guarantee.This should be investigated before promoting the feature out of
experimental.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.