Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display redacted strings in the client #63

Closed
wants to merge 3 commits into from

Conversation

MonkeyIsNull
Copy link
Contributor

@MonkeyIsNull MonkeyIsNull commented May 20, 2024

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.

@anewton1998
Copy link
Collaborator

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
Copy link
Contributor Author

Cool, I will rip the dbg stuff out, move the stuff around and then re-do a pull request

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.

2 participants