Remove MacroRef from user-facing APIs #937
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #937 +/- ##
==================================================
Coverage ? 69.99%
Complexity ? 6795
==================================================
Files ? 193
Lines ? 26876
Branches ? 4876
==================================================
Hits ? 18813
Misses ? 6565
Partials ? 1498 ☔ View full report in Codecov by Sentry. |
popematt
force-pushed
the
topo-sort-macros
branch
from
8418f16
to
0cc8455
Compare
popematt
force-pushed
the
topo-sort-macros
branch
from
0cc8455
to
d279ae1
Compare
| @@ -117,6 +117,9 @@ class IonReaderContinuableCoreBinary extends IonCursorBinary implements IonReade | |||
| // The core MacroEvaluator that this core reader delegates to when evaluating a macro invocation. | |||
| private MacroEvaluator macroEvaluator = null; | |||
|
|
|||
| // The encoding context (macro table) that is currently active. | |||
| private EncodingContext encodingContext = null; | |||
There was a problem hiding this comment.
🗺️ I moved EncodingContext out of MacroEvaluator since the evaluator no longer needs to look up macros by name/address.
| @@ -149,6 +149,7 @@ public IonWriter build(Appendable out) { | |||
| _Private_IonTextWriterBuilder_1_1 b = fillDefaults(); | |||
| ManagedWriterOptions_1_1 options = new ManagedWriterOptions_1_1( | |||
| false, | |||
| true, | |||
There was a problem hiding this comment.
🗺️ Whether to invoke macros by name in TDL. See ManagedWriterOptions_1_1.
| * Indicates whether a particular [SymbolToken] should be written inline (as opposed to writing as a SID). | ||
| * Symbols with unknown text must always be written as SIDs. | ||
| */ | ||
| private fun SymbolInliningStrategy.shouldWriteInline(symbolKind: SymbolKind, symbol: SymbolToken): Boolean { |
There was a problem hiding this comment.
🗺️ This was unused.
| @@ -139,6 +139,7 @@ public IonWriter build(OutputStream out) { | |||
| } | |||
| ManagedWriterOptions_1_1 options = new ManagedWriterOptions_1_1( | |||
| true, | |||
| false, | |||
There was a problem hiding this comment.
🗺️ Whether to invoke TDL macros by name (rather than by address). See ManagedWriterOptions_1_1.
| body = { string("abc") }, | ||
| expectedBody = "\"abc\"" |
There was a problem hiding this comment.
🗺️ Changed this so that it wouldn't have "foo" like so many of the symbols and macros.
There was a problem hiding this comment.
🗺️ This ended up being a large change as I ripped out all of the macro names and the MacroEvaluator no longer needs to be set up with a macro table, so I went ahead and converted it to use the expression dsl as well for (IMO) better readability.
popematt
changed the title
src/test/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1_Test.kt
Outdated
Show resolved
Hide resolved
Issue #, if available:
Follow up to #934 (comment)
Description of changes:
Removes
MacroReffrom theExpressionmodel and the macro writer APIs so that we don't have to deal with potentially stale references, and so that we can automatically add macro dependencies to the macro table.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.