Display redacted strings in the client #62
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm making this pull request mainly more of a code review. I have too many questions and it's easier to do this here rather than get on a call and try and sort it out.
First Question: where do you see this code going? At first I thought it would go into redacted.rs in the client, but then for some reason I can't remember I decided I didn't like that and put it into a utils.rs. I'm pretty sure I don't like it in utils and it should go into redaction.rs with the ToMd code .. .but again, I'll leave the code here so it gets your feedback and ideas.
Second Question: I'm not positive the spot in query.rs where I've put replace_redacted_items is the best spot. There might be a more obvious location that I'm missing? Let me know.
Third Question: utils.rs is heavily commented b/c I wasn't sure any of what I was doing would make sense to anyone else. Do you want all those stripped out?
Fourth Question: It also has a ton a debug statements left over so I can grok what's going on with the redactions when we attempt to do anything. Do you want all those stripped out or left in?
Fifth Question: Do you this code more streamlined, re-factored, anything else with it? I'm not in love with how it came out and I definitely don't like the names of any of the methods, and I've left the replacementValue in but commented out, so I'm open to changing anything and everything about it.
Sixth Question: More tests? Or is what we have here sufficient?
Anything else I've missed?