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

Add an inherent JsTaggedBase64::to_string method for JS bindings #13

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

jbearer
Copy link
Member

@jbearer jbearer commented Feb 25, 2022

No description provided.

@jbearer jbearer self-assigned this Feb 25, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1896257024

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.8%) to 89.333%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib.rs 0 3 0.0%
Totals Coverage Status
Change from base Build 1873605534: -1.8%
Covered Lines: 134
Relevant Lines: 150

💛 - Coveralls

//
// Note: this method is included for WASM bindings, since the trait methods from Display don't
// get compiled to WASM.
#[allow(clippy::inherent_to_string_shadow_display)]
Copy link
Contributor

@nyospe nyospe Feb 25, 2022

Choose a reason for hiding this comment

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

Oh, huh. We have https://github.com/SpectrumXYZ/tagged-base64/blob/0159640927e5e1bfeac431f121362c70f0ca7e94/src/lib.rs#L121 for the TaggedBase64 type, it's odd that we don't have the same for the JS Wrapper.

Is a method, as opposed to a standalone function, preferable here? Overloaded name aside?

It actually looks like the to_string function was added specifically for this purpose, before the JsTaggedBase64 proxy, and @pictographer forgot to change the parameter type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Incidentally... there's an outstanding issue around this, not a lot of activity. rustwasm/wasm-bindgen#2073

@pictographer pictographer merged commit 5fc88a5 into main Feb 25, 2022
@pictographer pictographer deleted the jeb-js branch February 25, 2022 17:33
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.

4 participants