Skip to content

Refactor redactor into replacer#2277

Merged
DrJosh9000 merged 1 commit into
mainfrom
redactor-refactor
Aug 9, 2023
Merged

Refactor redactor into replacer#2277
DrJosh9000 merged 1 commit into
mainfrom
redactor-refactor

Conversation

@DrJosh9000

@DrJosh9000 DrJosh9000 commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

This changes redactor into replacer. A summary:

  1. The minimum redaction length, ValuesToRedact, and VarsToRedact are moved into a new package, redact
  2. The Redactor and necessary friends are renamed and moved into replacer.
  3. pipeline_upload.go is changed to use a Replacer to search through the JSON in a streaming manner.
  4. The implementation of flushUpTo is changed to call a callback when processing matches instead of using a fixed []byte.
  5. flushUpTo also no longer eagerly flushes into the middle of matches.
  6. Basically everything else is mechanical.

if _, err := r.dst.Write(r.subst); err != nil {
// Now handle the match itself.
// Call the replacement callback to get a replacement.
if repl := r.replacement(r.buf[match.from:match.to]); len(repl) > 0 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is "replace the thing matched with an empty string" (ie delete the matched string) a valid use case? i can see the semantics there being a little confusing if the user tried to do that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be supported - replacement should return nil. The if on this line is just to skip calling r.dst.Write with nil or empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet as

@DrJosh9000

DrJosh9000 commented Aug 8, 2023

Copy link
Copy Markdown
Contributor Author

I'm going to add some more tests, since there is 0 coverage in most of clicommand.

Edit: Done.

@triarius triarius left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I think I know how to use this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants