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

fix: Base64 decoding to discard newline characters #1941

Merged
merged 7 commits into from
May 16, 2024
Merged

Conversation

suztomo
Copy link
Member

@suztomo suztomo commented May 15, 2024

Using b/274482271#comment3 by @eamonnmcmanus

  • Added test cases for new line characters.
  • This PR keeps the trim() method that was also added for the compatibility.

@suztomo suztomo requested a review from a team as a code owner May 15, 2024 19:18
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label May 15, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels May 15, 2024
Copy link

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with just one very small thing.

@@ -26,6 +26,10 @@
*/
@Deprecated
public class Base64 {
// Special decoders that discards the new line character so that the behavior matches what
// we had with Apache Commmons Codec's decodeBase64.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth commenting on what the 64 means. (It doesn't mean very much, since it's only used for encoding and we don't do that.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added explanation of the 2nd parameter of the withSeparator method.

Copy link

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for misunderstanding in my previous comment.

@suztomo suztomo merged commit 4e153db into main May 16, 2024
21 checks passed
@suztomo suztomo deleted the base64_encoding branch May 16, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants