Add variants without padding

[larskuhtz]

Section 3.2 of RFC 4648 notes that the padding of base64 encoded data may be omitted when the length of the encoded data is known implicitly. This is the case for many applications that require a textual encoding of binary data. Examples include JSON and URLs.

[bos]

I might not be opposed to this addition in principle, but on first glance this patch is mostly a vast amount of copypasta, and I will not accept a massive amount of purposeless code duplication.

The patch is also far too big to review, and offers no explanation for why there is so much code to achieve so little.

I would much rather see a series of small, understandable patches that make only the minimal changes necessary. I also don't see the point of adding more modules, so if you really need to do that, you'll want to explain your reasoning here. Thanks.

[larskuhtz]

I agree that the code duplication is not nice.

The reason why I added new modules is to follow the existing pattern of exposing different variants (lazy, URL) as separate module with a unified API (encode, decode, decodeLenient). Already now some modules contain mostly identical code (compare for example Data.ByteString.Base64.Lazy and Data.ByteString.Base64.URL.Lazy). I tried to closely following this pattern and avoided changing other modules. Some common code could be moved to Data.ByteString.Base64.Internal. There would still be quite a bit of boilerplate left.

I see three options:

1. moving as much common code as possible to Data.ByteString.Base64.Internal and reducing the exported modules to plain API boilerplate, or
2. changing the current API pattern by adding Boolean flags to the encode and decode functions for selecting the no-padding variants (this would break the existing API), or
3. changing the current API pattern by introducing new function names for selecting the no-padding variants (for instance encodeWithoutPadding and decodeWithoutPadding).

Let me know which one you prefer or if there is another option that I missed.

[xaviershay]

Any preference? #2 doesn't seem viable.

[sirlensalot]

Maintainers, any decision on how to proceed with this? This is a standard feature in Base64 libraries and I personally need it in my code. What is the way forward please?

1. Go with the patch author's approach. Unless someone can offer a more succinct way to present this as separate modules I disagree with the claim that this has "massive copypasta"; it is consistent with the "module-for-every-version" we see across the family of ByteString libraries. This could include more work per @larskuhtz's suggestion Bugfixes with respect to encoding table construction #1 but more specific direction is needed.

2. It does seem like suggestion support for base64url encoding #2 above isn't viable as noted by @xaviershay .

3. Add the variants in option cleanup of previous commit and some tests #3.

[23Skidoo]

The diff doesn't look that massive to me, though I'm all for moving more stuff to an .Internal module if possible. @hvr, your opinion?

[23Skidoo]

Re: 3, instead of {encode,decode}WithoutPadding it may make more sense to implement decodeWithOpts and encodeWithOpts taking

data Opts = Opts {
   optPadding :: Bool
   ...
}

as the first parameter to make the code more future proof.

[emilypi]

Is this going to go in at any point? 300 lines of code is definitely not too big of a diff. If There's not a chance of this going in I'll just release the library myself @bos @23Skidoo

[23Skidoo]

@bos's original objection was that the patch introduced excess copypasta. If the patch was reworked to remedy this (see above comments for suggestions), I think that the fixed version would be gladly accepted.

[23Skidoo]

Though if @hvr agrees, we may just merge it as is.

[23Skidoo]

Closing in favour of #26.