Nim icon indicating copy to clipboard operation
Nim copied to clipboard

Remove unused `base64.encode` overload

Open AmjadHD opened this issue 2 years ago • 6 comments

AmjadHD avatar Sep 16 '22 20:09 AmjadHD

Do we know why this procedure was added in the first place?

Varriount avatar Sep 19 '22 22:09 Varriount

There was the string overload, then the openarray overload was added in 6901a8cb13d19a3d251911af240c5f2a54809eba (https://github.com/nim-lang/Nim/pull/646), Ig the author wasn't aware that string is compatible with openArray[char].

AmjadHD avatar Sep 20 '22 12:09 AmjadHD

Is it ok to have an overload for integer types of arbitrary sizes. Shouldn't the input be byte strings.

AmjadHD avatar Sep 20 '22 12:09 AmjadHD

Please add a changelog entry, then this can be merged.

Is it ok to have an overload for integer types of arbitrary sizes. Shouldn't the input be byte strings.

Is this a question? And if so, what code are you referencing?

Varriount avatar Sep 20 '22 20:09 Varriount

Please add a changelog entry, then this can be merged.

Is this needed, from the user's perspective nothing changed.

Is it ok to have an overload for integer types of arbitrary sizes. Shouldn't the input be byte strings.

Is this a question? And if so, what code are you referencing?

Yes, https://github.com/nim-lang/Nim/pull/20369/files#diff-cc4f63a53ac195237326427c11b84c89967c417d474b4cd55ad54b48a09c9a1cL144. This overload takes arbitrary integer types and wraps the values that do not fit in 1 byte, Is this behaviour desired or should it error instead ?

AmjadHD avatar Sep 21 '22 05:09 AmjadHD

Can you give an example of what happens with 1-byte integers, vs multi-byte integers?

Varriount avatar Sep 21 '22 19:09 Varriount

import std/base64

var x = @[72, 101, 108, 108, 111] # Hello
let e = encode(x)
x[0] += 256
assert encode(x) == e

AmjadHD avatar Sep 21 '22 20:09 AmjadHD

Hm. I don't know. I would expect is one of the following:

  • When given a multi-byte integer, encode would output multiple characters.
  • encode would not accept multi-byte integers.

Both are breaking changes, though the current behavior is arguably a bug. I would implement #2 first, and see how many, if any, packages that breaks.

Varriount avatar Sep 21 '22 20:09 Varriount

I tried T: range[0..255]|uint8|char but this doesn't work even for encode [10].

AmjadHD avatar Sep 22 '22 08:09 AmjadHD

for reference, the API we use is: https://github.com/status-im/nim-stew/blob/master/stew/base64.nim (notably, there are many "base64" alphabet standards)

arnetheduck avatar Sep 23 '22 07:09 arnetheduck

Should I change T: byte|char then and deprecate the T: SomeInteger overload ?

AmjadHD avatar Sep 26 '22 11:09 AmjadHD

I tried T: range[0..255]|uint8|char but this doesn't work even for encode [10].

Why doesn't it work ?

AmjadHD avatar Sep 26 '22 11:09 AmjadHD

cc @Varriount ^.

AmjadHD avatar Oct 01 '22 22:10 AmjadHD

Instead of T: range[0..255]|uint8|char use T: byte|char

Araq avatar Oct 01 '22 22:10 Araq

@Araq, What about code like encode @[111, 113, 32] ? should it be deprecated ?

AmjadHD avatar Oct 01 '22 22:10 AmjadHD

@Araq is this fine ?

AmjadHD avatar Oct 02 '22 19:10 AmjadHD

@Araq, What about code like encode @[111, 113, 32] ? should it be deprecated ?

I think so but there is no RFC for it so better leave it as it is.

Araq avatar Oct 03 '22 04:10 Araq

Can this be merged now ?

AmjadHD avatar Oct 04 '22 12:10 AmjadHD

@Araq merge this please.

AmjadHD avatar Oct 08 '22 19:10 AmjadHD

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from 944e4cf5853ad12f0738900e224109bb321715d0

Hint: mm: orc; opt: speed; options: -d:release 162851 lines; 9.323s; 614.402MiB peakmem

github-actions[bot] avatar Oct 09 '22 20:10 github-actions[bot]