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 display trait to descriptor #559

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

reez
Copy link
Collaborator

@reez reez commented Jun 13, 2024

Description

Adds more to #551 , I was working on BDK iOS and just saw that I was using Descriptor.asString() so I wanted to make that nicer by using the display trait on Mnemonic instead now after seeing #551 .

This also removes the asString() method which is nice.

Notes to the reviewers

We still have the as_string_private(), I couldnt think of an immediately good way to change that, let me know if you know of one or I'll keep thinking on it to see if something strikes me; but I dont see any reason for that to hold this up even though the pairing of to_string and as_string_private might be a little awkward, we are already walking the path of using this new to_string from the Display trait so we should keep using it on types like this because its really nice.

Let me know any thoughts on that though-

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@reez reez force-pushed the descriptor branch 6 times, most recently from 293dc7a to 5015bc1 Compare June 13, 2024 13:08
@reez reez marked this pull request as ready for review June 13, 2024 13:28
@reez reez requested a review from thunderbiscuit June 13, 2024 13:28
@reez reez self-assigned this Jun 13, 2024
@thunderbiscuit
Copy link
Member

Hey thanks for this! The as_string_private() is just another method with a name that's close but nothing else IMO.

BUT you got me looking into it just to make sure this is what it was called on the Rust side and in fact in Rust that method is called to_string_with_secret(). Weird! Why did we not name it the same??? Git blame tells me it was ME who did that but surely I would not (I must have left my computer unattended at the library for 20 minutes and someone committed that).

So really we don't know who did this terrible mistake but I suggest we fix it here and name it to_string_with_secret(). What do you think?

@reez
Copy link
Collaborator Author

reez commented Jun 13, 2024

Hey thanks for this! The as_string_private() is just another method with a name that's close but nothing else IMO.

BUT you got me looking into it just to make sure this is what it was called on the Rust side and in fact in Rust that method is called to_string_with_secret(). Weird! Why did we not name it the same??? Git blame tells me it was ME who did that but surely I would not (I must have left my computer unattended at the library for 20 minutes and someone committed that).

So really we don't know who did this terrible mistake but I suggest we fix it here and name it to_string_with_secret(). What do you think?

Love it, will do that right now, thanks for looking at the rust side and the good suggestion

@reez
Copy link
Collaborator Author

reez commented Jun 13, 2024

Hey thanks for this! The as_string_private() is just another method with a name that's close but nothing else IMO.

BUT you got me looking into it just to make sure this is what it was called on the Rust side and in fact in Rust that method is called to_string_with_secret(). Weird! Why did we not name it the same??? Git blame tells me it was ME who did that but surely I would not (I must have left my computer unattended at the library for 20 minutes and someone committed that).

So really we don't know who did this terrible mistake but I suggest we fix it here and name it to_string_with_secret(). What do you think?

Hey thanks for this! The as_string_private() is just another method with a name that's close but nothing else IMO.

BUT you got me looking into it just to make sure this is what it was called on the Rust side and in fact in Rust that method is called to_string_with_secret(). Weird! Why did we not name it the same??? Git blame tells me it was ME who did that but surely I would not (I must have left my computer unattended at the library for 20 minutes and someone committed that).

So really we don't know who did this terrible mistake but I suggest we fix it here and name it to_string_with_secret(). What do you think?

this should be all set now @thunderbiscuit I made the change to_string_with_secret() 👍

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK a6826cf.

@reez reez merged commit e97e9b7 into bitcoindevkit:master Jun 13, 2024
25 checks passed
@reez reez deleted the descriptor branch June 13, 2024 15:15
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