[5/n] [test-utils] turn redactor into the builder pattern #6786
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
sunshowers
changed the base branch from
main
to
sunshowers/spr/main.5n-test-utils-turn-redactor-into-the-builder-pattern
| let mut redactor = Redactor::new(); | ||
| redactor.extra(redactions); |
There was a problem hiding this comment.
I don't have the context for where else you'll be using the redactor in your future PRs but it feels like having the builder methods take self instead of &mut self would be more ergonomic, at least in this case.
There was a problem hiding this comment.
Ah so an issue is that we have places where we do conditional redactions, and setting redactor = redactor.xyz() feels a bit weird to me. In general whenever there are conditional things I tend to prefer &mut self builders. But 🤷🏽
There was a problem hiding this comment.
Yeah I feel like neither is ever the "right" answer. (The one unfortunate thing about the builder pattern.)
| redactor.map_or_else( | ||
| || r.to_string(), | ||
| |redactions| redact_extra(r, redactions), | ||
| |redactor| redactor.do_redact(r), | ||
| ) |
There was a problem hiding this comment.
Something that comes to mind as I read this is that I think we can simplify usage quite a bit in this file if Option<Redactor> could be replaced with a Redactor that does nothing by default. I think #[derive(Default)] on Redactor would do the right thing?
There was a problem hiding this comment.
Yeah that's reasonable.
There was a problem hiding this comment.
Done. I think the default should be that redactions are enabled, so I added a manual Default impl as well as a noop constructor.
| pub fn do_redact(&self, input: &str) -> String { | ||
| // Perform extra redactions at the beginning, not the end. This is because | ||
| // some of the built-in redactions in redact_variable might match a | ||
| // substring of something that should be handled by extra_redactions (e.g. |
There was a problem hiding this comment.
[bikeshed 2/2]:
| pub fn do_redact(&self, input: &str) -> String { | |
| // Perform extra redactions at the beginning, not the end. This is because | |
| // some of the built-in redactions in redact_variable might match a | |
| // substring of something that should be handled by extra_redactions (e.g. | |
| pub fn redact(&self, input: &str) -> String { | |
| // Perform extra redactions at the beginning, not the end. This is because | |
| // some of the built-in redactions in redact_variable might match a | |
| // substring of something that should be handled by extra_redactions (e.g. |
There was a problem hiding this comment.
I had redact at first, but especially now that the config knobs are shorter (e.g. uuids, basic) I wanted to draw a distinction between the knobs and the actual process of redaction.
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
sunshowers
changed the base branch from
sunshowers/spr/main.5n-test-utils-turn-redactor-into-the-builder-pattern
to
main
|
Thanks iliana! |
sunshowers
deleted the
sunshowers/spr/5n-test-utils-turn-redactor-into-the-builder-pattern
branch
In an upcoming PR I want to redact everything except UUIDs. The most convenient
way to do that is to use the builder pattern for the redactor.