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.
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
feat: show match context #396 #436
base: master
Are you sure you want to change the base?
feat: show match context #396 #436
Changes from 5 commits
1d691d1
7853efc
ba54a3a
a485575
8dd4305
7d08b6d
368c733
2326e80
c5edd00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
same goes for this function. I suggest
sanitize
andsanitize(atob(str))
instead ofbase64ToUtf8(str)
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.
As mentioned above, I think that
sanitize
is too vague. If there will be another use case for such decryption in the project, it would be easier to get to know what was it's original purpose.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.
On the other hand, base64ToUtf8 is wrong:
I suggested
sanitize
because it's short, but it's true that it's a bit vogue . Consider:safeText(text)
,toSafeText(text)
,toPrintableString(text)
,replaceUnprintableCharacters(text)
, as an alternative tosanitize
base64ToSafeText(text)
,base64ToSafeText(text)
,base64ToPrintableString(text)
,base64DecodeAndReplaceUnprintableCharacters(text)
as an alternative tobase64ToUtf8
(or another variation that describes "replacing unprintable characters in a string with dots" succintly)
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.
I just noticed in the source that the name is already
base64ToSanitizedUtf8
. That's much better (though utf8 is still technically incorrect - maybe text or string?)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.
maybe remove the whitespace between letters? So
(" ")
. I understand that now spacing depends on this, but this should be solved anyway.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.
Done, now there's no spaces in between bytes or chars.
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.
This time I was probably unclear. The usual hexdump format, that it would be nice to have:
The important parts are:
So spaces between bytes are OK (they make bytes easier to read). And the really important thing is 16 bytes per line. It may sound like I'm nitpicking, but I'm not ;)
As a POC:
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.
To I think this is the required change, but please verify: