Skip to content

Fix base32 unicode alphabet#2380

Open
loki1205 wants to merge 3 commits into
gchq:masterfrom
loki1205:fix-base32-unicode-alphabet
Open

Fix base32 unicode alphabet#2380
loki1205 wants to merge 3 commits into
gchq:masterfrom
loki1205:fix-base32-unicode-alphabet

Conversation

@loki1205
Copy link
Copy Markdown

Description
Provide a description of the pull request and the changes that it makes.

Existing Issue
#2355

Screenshots
Screenshot 2026-05-13 at 11 30 09 PM

AI disclosure
AI Tools used:

  • ChatGPT
  • Github Copilot (Auto Agent)

Regardless of AI tool usage, I take full ownership of the code. This is my first open source bug fix and I might have made some mistakes, if so, kindly feel free to add comment, I will be very happy to fix the comment. But, I fully understand the logic.

Test Coverage
Done, I found 3 files with the usage of the method, I have updated tests in those files.

@loki1205
Copy link
Copy Markdown
Author

Hi, @C85297 , This is my first time merging a commit to an opensource codebase, so I am a little nervous, eventhough I have been working as a fullstack developer for past 3 years, this feels like an "uncharted territory". I had read Contributing.md doc and Wiki pages, and tried to make everything as per the docs, but if I have missed something in any case, kindle feel free to enlighten me, I will definitely fix it.

The same goes for the given fix, I took help from AI to fix this, but I have both verified the code and tested the logic manually for different cases and it looks good. Would love to hear from you :) .

Thank you for the opportunity!

C85297
C85297 previously approved these changes May 14, 2026
Copy link
Copy Markdown
Member

@C85297 C85297 left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks @loki1205 ! Really appreciate the added tests.

I just want to raise that this issue continues to exist within From Base32, and the other base(n) encoders/decoders, in case you want to take a stab at any of them within the same pull request.
Either way, this pull request fixes the issue, so happy for it to be merged as-is.

Also, thank you for the AI disclosure, which is really appreciated. Unfortunately, we are having to perform some internal work to update our policies and Contributor License Agreement before we can merge external AI-contributions. Please do not take this as a reflection of your work, its merely our internal bureaucracy. I hope this will be unblocked soon so that we can merge this pull request.

@loki1205
Copy link
Copy Markdown
Author

loki1205 commented May 14, 2026

Thanks @C85297 , I am glad that you approved this.

And, I understand AI policy, I also saw other PRs in the repo which were not merged.

Also, I saw the issue in From Base 32 and other base n code and I left them because I thought to fix only one bug per PR. It is easy to review and maintain. Hence, It would be great if you can raise a bug for the other scenarios.

Now, I can do it without AI because I have understood the fix. :)

Thanks again! My first contribution was a success 🥳

edit: there are some checks failing (Lint), can I continue to fix that or do I need to wait?

edit again: fixed lint issues, and ran npm run lint along with npm run test. Both of them are passing now. So, it should do in PR also 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants