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

feat(encoding/unstable): decodeBase32Hex() and encodeBase32Hex() #4931

Merged
merged 19 commits into from
Aug 9, 2024

Conversation

BenMcLean981
Copy link
Contributor

@BenMcLean981 BenMcLean981 commented Jun 2, 2024

As requested in #4917 I have added base32hex encoding and decoding.

I first created new tests from the base32 test cases and using this tool:
https://tomeko.net/online_tools/base32hex.php?lang=en

Once those tests passed, I extracted the common base32 and base32 hex into a _utilsmodule with a new parameter for a lookup table.

This is my first time contributing to an open source project and using deno. I am having trouble running the task ok. It is failing on my documentation examples with the error below. I think I'm missing a step though, because it's executing with the published module from jsr rather than the local version. This means it makes sense that my module is not there, because it won't be in version 0.224.3. If there's any step I'm missing here I'd really appreciate any feedback.

[error] Failed to execute snippet: 
import { decodeBase32Hex } from "@std/encoding/base32hex";
import { assertEquals } from "@std/assert/assert-equals";

assertEquals(
  decodeBase32Hex("GZRTMMDDGA======"),
  new TextEncoder().encode("6c60c0"),
);
error: Unknown export './base32hex' for '@std/encoding@0.224.3'.
  Package exports:
 * .
 * ./ascii85
 * ./base32
 * ./base58
 * ./base64
 * ./base64url
 * ./hex
 * ./varint
 at file:///home/benmclean/projects/deno/deno_std/encoding/base32hex.ts:54
[error] Failed to execute snippet: 
import { encodeBase32Hex } from "@std/encoding/base32hex";
import { assertEquals } from "@std/assert/assert-equals";

assertEquals(encodeBase32Hex("6c60c0"), "GZRTMMDDGA======");
error: Unknown export './base32hex' for '@std/encoding@0.224.3'.
  Package exports:
 * .
 * ./ascii85
 * ./base32
 * ./base58
 * ./base64
 * ./base64url
 * ./hex
 * ./varint
 at file:///home/benmclean/projects/deno/deno_std/encoding/base32hex.ts:74
2 errors found

Closes #4931

@BenMcLean981 BenMcLean981 requested a review from kt3k as a code owner June 2, 2024 21:14
@CLAassistant
Copy link

CLAassistant commented Jun 2, 2024

CLA assistant check
All committers have signed the CLA.

@BenMcLean981 BenMcLean981 changed the title feat(encoding) Added base32 hex decode and encode. feat(encoding): Added base32 hex decode and encode. Jun 2, 2024
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Thank you for this. @std/encoding is about to be in its release candidate phase, so it'll be in a change freeze (except for documentation improvements and bug fixes). This change will be merged once that's complete.

assert/assert_almost_equals.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the assert label Jun 3, 2024
@iuioiua iuioiua changed the title feat(encoding): Added base32 hex decode and encode. feat(encoding): decodeBase32Hex() and encodeBase32Hex() Jun 3, 2024
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Can you please add the new module to the exports in encoding/deno.json?

@BenMcLean981
Copy link
Contributor Author

Can you please add the new module to the exports in encoding/deno.json?

I actually already did, I didn't add the _utils.ts module though because it should be private.

@kt3k kt3k added the after 1.0 label Jun 6, 2024
@iuioiua iuioiua self-requested a review June 6, 2024 20:13
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.88%. Comparing base (714fccb) to head (6cccaaf).
Report is 60 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4931      +/-   ##
==========================================
- Coverage   96.37%   95.88%   -0.49%     
==========================================
  Files         465      475      +10     
  Lines       37506    38464     +958     
  Branches     5527     5553      +26     
==========================================
+ Hits        36147    36883     +736     
- Misses       1317     1539     +222     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BlackAsLight
Copy link
Contributor

The only error left seems to be that you forgot to update the assertEquals in the jsdoc examples when you copied it over.

@BenMcLean981
Copy link
Contributor Author

The only error left seems to be that you forgot to update the assertEquals in the jsdoc examples when you copied it over.

Good catch, this has been fixed, thanks.

@iuioiua
Copy link
Contributor

iuioiua commented Aug 1, 2024

Hey there. Sorry for the delay. On second thought, with the only difference being the alphabet used, I think we should add an options argument to the base32 functions, instead of creating new symbols and modules. We can then just use a simple ternary operator in the function bodies to choose the appropriate alphabet. It could look like this.

interface Base32Options {
  /**
   * Base32 format to use.
   *
   * @default {"standard"}
   */
  format?: "standard" | "hex";
}

What do y'all think?

@iuioiua
Copy link
Contributor

iuioiua commented Aug 1, 2024

Actually, we can have separate functions but have them all in the existing base32 module, since there's so much overlap.

@kt3k kt3k changed the title feat(encoding): decodeBase32Hex() and encodeBase32Hex() feat(encoding/unstable): decodeBase32Hex() and encodeBase32Hex() Aug 1, 2024
@BlackAsLight
Copy link
Contributor

I think the best way to do it would be the same way base64 and base64url are done.

@iuioiua
Copy link
Contributor

iuioiua commented Aug 1, 2024

base32 and base32Hex functions have almost complete overlap - there's no benefit to separating them. Arguably, the same could be said for base32Url, but that's a separate conversation.

@BlackAsLight
Copy link
Contributor

I don't have an opinion one way or the other on how it should be done, but I think consistency is best.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I chatted with Yoshiya, and on second thought, we're happy to proceed with this format. I also made a couple of tweaks. Now, LGTM! Thank you very much.

@iuioiua iuioiua merged commit 96da26c into denoland:main Aug 9, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants