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
Simplify MakePrintable by using AND and avoiding converting to string #2101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please provide a unit test that helps ensure this won't cause a regression?
I really appreciate your attention to detail and am excited to have you helping!
@tig -- added two parameterized tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I accept this, I'd like confirmation that this change has been thoroughly tested on Windows, Linux, and the Mac.
I don't fully understand the code paths/scenarios involved where this method is used and am nervous about introducing a regression in an area where we have really poor unit testing (all drivers).
It's a straightforward method. Unit tests cover all scenarios that would be there. I don't think deeper testing is needed for this particular change. |
I confirm that the method is common for all drivers and this is a short way to get the hexadecimal from the rune which is well written. Thanks @pavkam. |
This PR simplifies MakePrintable by using simpler bitwise OR instead of string manipulation.
Pull Request checklist:
CTRL-K-D
to automatically reformat your files before committing.dotnet test
before commit///
style comments)