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 #62

Closed
wants to merge 1 commit into from

Conversation

MonkeyIsNull
Copy link
Contributor

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?

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.

1 participant