Implements make_symbol system macro
#868
Conversation
| // the `make_value_writer` method. | ||
| /// A writer which can encode nested values. | ||
| /// | ||
| /// Implementors include top-level writers, container writers, and e-expression writers. |
There was a problem hiding this comment.
🗺️ I had this doc comment in my workspace as part of #867 but didn't push it. 🤦 Slipping it into this PR.
src/lazy/expanded/macro_evaluator.rs
Outdated
| /// Evaluation logic shared by the `make_string` and `make_symbol` macros. | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub struct MakeStringExpansion<'top, D: Decoder> { | ||
| pub struct MakeTextExpansion<'top, D: Decoder> { |
There was a problem hiding this comment.
🗺️ I extracted the logic common to both make_string and make_symbol into its own type.
There was a problem hiding this comment.
Do you need to have separate implementations that extend/wrap this? The only difference is at the end where it constructs either a Symbol or a String. Could you just have two different constructors that set it up to use either
|constructed_text| ValueRef::String(StrRef::from(constructed_text)) or |constructed_text| ValueRef::Symbol(SymbolRef::from(constructed_text)) depending on which one you need?
There was a problem hiding this comment.
Yeah, I could do that. I was trying to keep the doc comments for make_string, but I guess that's not worth too much.
src/lazy/expanded/macro_evaluator.rs
Outdated
| context: EncodingContextRef<'top>, | ||
| _environment: Environment<'top, D>, | ||
| ) -> IonResult<MacroExpansionStep<'top, D>> { | ||
| let constructed_text = self.arguments.concatenate_text_arguments(context)?; |
There was a problem hiding this comment.
🗺️ make_string now just calls MakeTextExpansion::concatenate_text_arguments and then massages the resulting &'bump str into a ValueRef.
src/lazy/expanded/macro_evaluator.rs
Outdated
| context: EncodingContextRef<'top>, | ||
| _environment: Environment<'top, D>, | ||
| ) -> IonResult<MacroExpansionStep<'top, D>> { | ||
| let constructed_text = self.arguments.concatenate_text_arguments(context)?; |
There was a problem hiding this comment.
🗺️ The new make_symbol impl calls MakeTextExpansion::concatenate_text_arguments and then massages the resulting &'bump str into a ValueRef.
| 'first name' | ||
| 'Hello, world!' | ||
| "#, | ||
| ) |
There was a problem hiding this comment.
🗺️ There are many corner cases untested here (empty args, discarding annotations, etc) as I believe ion-tests covers them.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
==========================================
+ Coverage 77.84% 77.86% +0.01%
==========================================
Files 136 136
Lines 34235 34273 +38
Branches 34235 34273 +38
==========================================
+ Hits 26650 26685 +35
- Misses 5629 5631 +2
- Partials 1956 1957 +1 ☔ View full report in Codecov by Sentry. |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.