awslabs
Make FormatOptions immutable
Description of changes
This makes the FormatOptions immutable, allowing us to store it if we want and re-use it. I also think it makes it clearer what is an override versus the user intention. Let me know if you like the pattern, I am not sure.
Checklist
- [x] Created unit tests in
tests/unitand/or in Rust for my feature if needed - [x] Ran
make fixto format JS and apply Clippy auto fixes - [x] Made sure my code didn't add any additional warnings:
make check - [x] Added relevant type info in
types/directory - [x] Updated documentation if needed (API.md/README.md/Other)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
This is just my personal opinion without any consideration of performance or memory usage. I don't really like the approach of passing options and options_override between multiple functions. Would it be okay to clone options (and change their value) only if a change is needed?
#[derive(Clone)] // add
pub struct FormatOptions<'js> {
...
}
pub fn build_formatted_string<'js>(
result: &mut String,
ctx: &Ctx<'js>,
args: Rest<Value<'js>>,
options: &FormatOptions<'js>, // Mutable -> Immutable can be changed
) -> Result<()> {
let color = options.color; // add
let object_filter = options.object_filter; // add
let mut options = options.clone(); // add
...
// Reset to the initial value at the appropriate place in the loop:
options.color = color;
options.object_filter = object_filter;
// And change the parameters of the following function to pass by reference:
format_raw_string(result, str, &options);
format_raw(result, value, &options)?;
}
I think this approach is effective if you only need to make the parameters of functions exposed to the outside world immutable.
@Sytten @nabetti1720 i agree. It’s a bit clunky to pass both
I think this approach is effective if you only need to make the parameters of functions exposed to the outside world immutable.
The goal was to avoid the clone for each format essentially, otherwise I would probably just pass it as an owned value instead. @richarddavison
The goal was to avoid the clone for each format essentially, otherwise I would probably just pass it as an owned value instead. @richarddavison
Can you refactor to owned value instead? If not possible, Rc is also fine