-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
Add Base32 & Base64 on encodeString #2016
Conversation
I guess it could be done as follows, idk encodeString("base", string, {radix = 32}) |
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.
Thanks for the pull request! Seems good, I haven't tested this yet but will do so soon.
I think base32 and base64 is better for now. |
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.
Looks good, just some minor changes.
In general we might want to look into making Encode/DecodeString
a bit less repetitive. But don't bother with it for this PR.
TEA | ||
TEA, | ||
BASE32, | ||
BASE64 | ||
}; |
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.
The StringEncryptFunction
name is a bit odd due to base64
not being an encryption method. Maybe we should change this to StringEncodeFunction
?
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.
Thanks for review. I'll edit.
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.
@cleopatradev This change is still pending.
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.
@cleopatradev This is still an issue.
base32 should have |
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.
lgtm.
Exactly what is needed for this to switch from draft to ready for more reviews or merging? The PR was changed to draft status by @patrikjuvonen, maybe clarify the reason or we can mark it as ready for review again. |
sbx's request for change is still pending at #2016 (comment) |
Wrong branches, wait the new PR |
Fixes #2002
Adding encodeString base32&base64
Example:
When the PR is approved, I will update it on the wiki.