Skip to content

Conversation

rrrooommmaaa
Copy link
Contributor

This PR does a small fix


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)
  • Difficult to test (explain why)
  • Not worth testing
  • Tests will be added later (issue #...)
  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

return Buf.fromBase64Str(b64UrlStr.replace(/-/g, '+').replace(/_/g, '/'));
};

// todo: deprecate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this function and suggest to remove it. As in some places we might want to simplify code and use plain Uint8Array objects, and Uint8Array.toString() returns a totally different thing. (It's also hard to find usages of this function among toString() calls on other types of objects)
I suggest to explicitly use toUtfStr method)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one sounds good too 👍

@rrrooommmaaa rrrooommmaaa requested a review from sosnovsky March 13, 2023 08:55
sosnovsky
sosnovsky previously approved these changes Mar 13, 2023
@tomholub
Copy link
Collaborator

@rrrooommmaaa is this ready for merging?

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review March 16, 2023 09:39
@rrrooommmaaa rrrooommmaaa enabled auto-merge (squash) March 16, 2023 09:40
@rrrooommmaaa rrrooommmaaa requested a review from sosnovsky March 16, 2023 09:41
@rrrooommmaaa rrrooommmaaa merged commit d674533 into master Mar 16, 2023
@rrrooommmaaa rrrooommmaaa deleted the issue-0000-niceties branch March 16, 2023 10:31
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.

3 participants