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

restructure character validity check to be more readable and correct #1

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

endigma
Copy link
Contributor

@endigma endigma commented Jan 10, 2025

  • drops the 2-armed switch statement in favor of if
  • adds check that the key doesn't contain non-crockford32 runes

I'd also recommend adding test coverage for the validity fail branch of Decode() and perhaps running a full validation instead of only checking length. It looks like it would likely panic or fail with undefined behavior as the error result from crock32.Decode() is disregarded on L126.

@jackspirou
Copy link
Member

Thanks @endigma - LGTM!

@jackspirou jackspirou merged commit f5b0ed5 into agentstation:master Jan 10, 2025
1 check passed
@endigma endigma deleted the endigma/cleanup-isvalid branch January 10, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants