anewton1998
commented
5 months ago I definitely think it should go in redacted.rs and not utils.rs.
Regarding the db! question, this is from the Rustdoc on it:
The dbg! macro works exactly the same in release builds. This is useful when debugging issues that only occur in release builds or when debugging in release mode is significantly faster.
Note that the macro is intended as a debugging tool and therefore you should avoid having uses of it in version control for long periods (other than in tests and similar). Debug output from production code is better done with other facilities such as the debug! macro from the log crate.
So rip it out.
MonkeyIsNull
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? I'm also wary of the spec changing underneath us at the last moment and then not having them.
Fifth Question: Do you want 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. I've also left the code for 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? I've looked at it for so long I think I've become blind to the issues and have certainly overlooked more than a few things.