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

21870-Fix-Base64-madness-for-good #1331

Merged
merged 3 commits into from
May 11, 2018
Merged

21870-Fix-Base64-madness-for-good #1331

merged 3 commits into from
May 11, 2018

Conversation

svenvc
Copy link
Contributor

@svenvc svenvc commented May 10, 2018

https://pharo.manuscript.com/f/cases/21870/Fix-Base64-madness-for-good

Reimplement ByteArray>>#base64Encoded using ZnBase64Encoder and update comment

Reimplement String>>#base64Decoded using ZnBase64Encoder and update comment - changing the return type from String to ByteArray

Remove String>>#base64Encoded

Fix Base64 users in image

Add Base64Tests to explicitly cover #base64Encoded and #base64Decoded

Sven Van Caekenberghe added 3 commits May 10, 2018 19:11
…e comment

Reimplement String>>#base64Decoded using ZnBase64Encoder and update comment - changing the return type from String to ByteArray

Remove String>>#base64Encoded
Copy link
Contributor

@dionisiydk dionisiydk left a comment

Choose a reason for hiding this comment

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

Very good.
But could you explain difference between:

aString base64Decoded

and

ZnUtils decodeBase64: aString

Why in some places you use first version but in other places you use utils version?

Copy link
Collaborator

@akgrant43 akgrant43 left a comment

Choose a reason for hiding this comment

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

+1 to Denis comment (I assume it is for the case where you really do want a string back after decoding the base 64).

It would be good to add comments to ZnUtils class>>en/decodeBase64:, and maybe to #base64Decoded / #base64Encoded pointing users to ZnUtils

@svenvc
Copy link
Contributor Author

svenvc commented May 10, 2018

Excellent question, Denis !

ZnUtils class>>en/decodeBase64: act like the old methods (String<->String) but with an explicit use of ZnNullEncoder to indicate better what is happening - some internet protocols, like HTTP, use this in some places.

It was not strictly necessary to go via ZnUtils, but I felt it was appropriate in those few cases.

@svenvc
Copy link
Contributor Author

svenvc commented May 10, 2018

And yes, Alistair, an extra comment could help. I will try to remember to add it.

@MarcusDenker MarcusDenker merged commit 99a8c3f into pharo-project:development May 11, 2018
@svenvc
Copy link
Contributor Author

svenvc commented May 11, 2018

Thanks

@dionisiydk
Copy link
Contributor

I have impression that it is better to write such string conversion just in place and avoid ZnUtils class:

aString decodedBase64 asString

It really shows the purpose in places like http where it is needed. But ZnUtils version only raises questions like "Why use utils instead of string method?".

ZnUtils decodeBase64: aString

It provides zero information about expected logic and expected value.
And generally it is confusing to see utils in one place in system and string based code in another place.

So I would deprecate these utils methods. Besides both expressions have equal size.

@dionisiydk
Copy link
Contributor

Also using #asString in such "protocol" places (like http) is not nice. It would be better to introduce explicit asAsciiString and use it instead of #asString where we really need ascii string.

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.

4 participants