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

proposal: encoding/base64: add Encoding.EncodeString to match DecodeString #51295

Closed
andrewhodel opened this issue Feb 21, 2022 · 36 comments
Closed

Comments

@andrewhodel
Copy link

The DecodeString() method exists as a single function, an EncodeString() method should be added as it will not break forward compatibility and conform to a standard naming structure and module offering.

https://pkg.go.dev/encoding/base64

This is not a duplicate of a bug, I was asked to make it a proposal regardless of it being a bug or proposal.

This is a proposal in writing to a software repository.

@gopherbot gopherbot added this to the Proposal milestone Feb 21, 2022
@changkun changkun changed the title proposal: affected/package: missing function in encode/decode pair proposal: encoding/base64: missing function in encode/decode pair Feb 21, 2022
@klauspost
Copy link
Contributor

DecodeString is (b64 encoded string) -> (unencoded []byte).

The reverse is EncodeToString, which does (unencoded []byte) -> (b64 encoded string).

I am not sure what else you are looking for.

@seebs
Copy link
Contributor

seebs commented Feb 21, 2022

Is the function EncodeToString the behavior you want, and you think it should be named EncodeString?

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 21, 2022
@ianlancetaylor
Copy link
Contributor

What is the exact API that you are proposing?

We currently have

// DecodeString returns the bytes represented by the base64 string s.
func (enc *Encoding) DecodeString(s string) ([]byte, error)

// EncodeToString returns the base64 encoding of src.
func (enc *Encoding) EncodeToString(src []byte) string

What are you suggesting that we add? Thanks.

@andrewhodel
Copy link
Author

@ianlancetaylor

Do you not understand that the input arguments to those functions are different?

If there is an Encode function there should be a Decode function with the same name structure and:

  • the same input argument types
  • the same output argument types

I realize what happened, someone was building it and needing to encode a byte array to a string so they wrote that function, then needed to decode a string to a byte array so they wrote that function.

When people read the documentation first, they are confused by the lack of a common naming structure and input/output type structure.

That should be cleaned and reorganized so that when people are working and reading the 1000's of Go functions (certainly impossible to remember all) they will not get frustrated by constant flipping between string and []byte and a lack of a naming structure that can be quickly remembered because it adheres to both common function names per functionality and input/output type.

@andrewhodel

This comment was marked as off-topic.

@randall77
Copy link
Contributor

@andrewhodel Help us out a bit if you want us to consider your change. Please tell us exactly what method you want added. You've more than adequately defined the motivation - but we're still not clear on what exactly the changes you're proposing are. Write us a prototype of the function/method you want, and a 1-2 sentence godoc description of what it does. Thanks.

@andrewhodel
Copy link
Author

@randall77

EncodeString() and DecodeString() should both exist because one of them does and the concept of encoding/decoding is fundamentally bound together.

I didn't read every function, but you should and you should adhere this naming structure and ensure that all relevant sets exist.

In other words

Encode([]byte) means there should be Decode([]byte)

function_1_group_with_3_total() should include function_2_group_with_3_total() and function_3_group_with_3_total() and so on. For example, with hash functions there is usually a standard naming format due to the required functions, that of input, parse, decode, inputMore etc.

Then when someone is working for example with the encoding/base64 library, they can set a number of days or hours aside to work with the library then get used to the functions. In other words, they can type Encode and Decode and EncodeString and DecodeString enough times to not need to deal with remembering arguments to every single function instead only needing to remember the top level function names as they all follow a common naming pattern.

@randall77
Copy link
Contributor

Ok, but you still haven't answered my question - what exactly are you proposing to add to encoding/base64?

@andrewhodel
Copy link
Author

@randall77

I answered your question in the first sentence of the original reply to your original comment/question.

EncodeString() and DecodeString() should both exist because one of them does and the concept of encoding/decoding is fundamentally bound together.

These are functions in Go of the encoding/base64 library. One exists, one doesn't. Again, these are functions in Go.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Feb 22, 2022

@andrewhodel That is not an answer to the question that @randall77 asked and it is not an answer to the question I asked.

I will repeat:

We currently have

// DecodeString returns the bytes represented by the base64 string s.
func (enc *Encoding) DecodeString(s string) ([]byte, error)

// EncodeToString returns the base64 encoding of src.
func (enc *Encoding) EncodeToString(src []byte) string

What are you suggesting that we add? Please write it in the exact form that I wrote it. This is a programming language.
Precision matters. Please write down exactly what you mean. Don't describe what you mean in words. Write it in Go. Thanks.

@andrewhodel
Copy link
Author

func (enc *Encoding) EncodeString(s string) ([]byte, error)

You should also read and make sure all the other pairs exist for encoding and decoding of, with respect to both input and output:

  • byte arrays
  • strings

If you want me to write it, then provide commit access.

@ianlancetaylor

This comment was marked as off-topic.

@andrewhodel

This comment was marked as off-topic.

@ianlancetaylor

This comment was marked as off-topic.

@andrewhodel

This comment was marked as off-topic.

@ianlancetaylor

This comment was marked as off-topic.

@ianlancetaylor ianlancetaylor changed the title proposal: encoding/base64: missing function in encode/decode pair proposal: encoding/base64: add Encoding.EncodeString to match DecodeString Feb 22, 2022
@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 22, 2022
@andrewhodel

This comment was marked as off-topic.

@ianlancetaylor

This comment was marked as off-topic.

@andrewhodel

This comment was marked as off-topic.

@randall77
Copy link
Contributor

I don't see why EncodeString should return an error. The other Encode functions do not. Base64 encoding and decoding are not symmetric in that encoding should always succeed, whereas decoding might not.

Other than that, seems like a reasonable function to have. Do you have a situation where you have a string source and wish to encode it to a []byte output? Just wondering if this is a function you actually ran into a need for, or if you just were confused about the lack of a symmetric API.

@ianlancetaylor
Copy link
Contributor

In any case, thanks for defining precisely what you suggest that we add (although you omitted the doc comment). I gather you are thinking of something along the lines of

// EncodeString returns the base64 encoding of s.
func (enc *Encoding) EncodeString(s string) ([]byte, error) {
    buf := make([]byte, enc.EncodedLen(len(s)))
    enc.Encode(buf, []byte(src))
    return buf, nil
}

I think the argument that you are making is that this is parallel to the existing DecodeString method.

How often does a need for this method come up in practice? Do you have any examples of code that would use it? Thanks.

@andrewhodel

This comment was marked as off-topic.

@andrewhodel
Copy link
Author

andrewhodel commented Feb 22, 2022 via email

@ianlancetaylor

This comment was marked as off-topic.

@ianlancetaylor
Copy link
Contributor

Yes you need an error, what if the memory bound is reached while reading?

If a Go program runs out of memory, it will crash. There is no way for a function to return an error because it has run out of memory.

@ianlancetaylor

This comment was marked as off-topic.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Feb 22, 2022
@andrewhodel
Copy link
Author

andrewhodel commented Feb 22, 2022 via email

@andrewhodel

This comment was marked as off-topic.

@andrewhodel
Copy link
Author

andrewhodel commented Feb 22, 2022 via email

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

The current helper methods assume that base64 is applied to binary data, which is almost always a []byte.
And it is applied to create text data, which is often a string.
So the API uses []byte for the binary (unencoded) data, and string for the text (encoded) data.
This has worked very well in practice.
There is no obvious reason we should add the other pairs, which would at the very least confuse me when reading the API.

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Likely Decline in Proposals (old) Feb 23, 2022
@andrewhodel
Copy link
Author

I guess every middleman your Go servers work with support perfect utf8.

I need to send data to things so old that anything beyond 127 bits is out of the question so they can pass it on and then without base64 what are you going to do?

I know, another go type modification is just what the doctor called for!

@andrewhodel
Copy link
Author

@rsc it's simple, you have a string and a validation function that ensures that string is "at protocol" meaning hasn't the distant device doesn't even fully support ASCII, then you need to pass it through that distant device so you base64 encode the validated string and send it.

What you are saying isn't real, there's no where on earth where an operating system requires code to not using bits and those work with Go servers that use strings for simplicity.

@klauspost
Copy link
Contributor

@andrewhodel I think it would help a lot if you made an example on https://go.dev/play/ to show your use-case and why this isn't trivially covered by the existing API.

@andrewhodel
Copy link
Author

// working with string
var s = "asdfStringWithoutUnicodeMoreThanOneTwentySeven";
function_without_string_support([]byte(s));

Code reviewer: "That is messy code, there are too many type modifications".

I guess they will never figure it out, oh well poof.

@golang golang locked as too heated and limited conversation to collaborators Feb 24, 2022
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 2, 2022
@rsc
Copy link
Contributor

rsc commented Mar 2, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

No branches or pull requests

7 participants