Skip to content

Impl string conversion traits for CommandString#1137

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
Kixunil:try-from-command-string
Jul 29, 2022
Merged

Impl string conversion traits for CommandString#1137
apoelstra merged 1 commit intorust-bitcoin:masterfrom
Kixunil:try-from-command-string

Conversation

@Kixunil
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil commented Jul 27, 2022

After MSRV bump try_from usually refers to TryFrom method but
CommandString used inherent one making it confusing and non-idiomatic.

This implements FromStr and TryFrom<{stringly type}> for
CommandString and deprecates the inherent method in favor of those.
To keep the code using &'static str efficient it provides
try_from_static inherent method that only converts from
&'static str.

Closes #1135

@Kixunil Kixunil added enhancement API break This PR requires a version bump for the next release P-high High priority code quality Makes code easier to understand and less likely to lead to problems 1.0 Issues and PRs required or helping to stabilize the API labels Jul 27, 2022
@Kixunil Kixunil added this to the 0.29.0 milestone Jul 27, 2022
@Kixunil Kixunil mentioned this pull request Jul 27, 2022
3 tasks
@apoelstra
Copy link
Copy Markdown
Member

I'm tempted to just drop the existing try_from, impl TryFrom<Cow<'static, str>, and impl'ing the other TryFroms off of that.

Yes, this would be a breaking change in a single major rev, but it's so slight (since we have explicitly covered all the common examples of Into<Cow<'static, str>>) that I think we can get away with it.

@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Jul 28, 2022

I did consider it however if we ever want to provide TryFrom<Cow<'a, str>> (non-static lifetime) all 'static uses will start to silently allocate. Specialization on 'static lifetime is impossible and will not be possible anytime soon since lifetime generics don't monomorphise. I did think about these things in the past and came to conclusion that providing _static methods is the least bad approach. It also expresses the intent cleanly.

I was also thinking about removing the method instead of deprecating/ Chose deprecation because it is nicer to people. I'd only keep it for one release - we could remove it from master immediately after release.

@Kixunil Kixunil force-pushed the try-from-command-string branch from 59bd618 to 6fc0e9c Compare July 28, 2022 12:45
@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Jul 28, 2022

Added since to deprecated

@apoelstra
Copy link
Copy Markdown
Member

Ok, I understand. I thought the _static method was purely a contortion to allow deprecation. But if it's to avoid specializing on 'static which is impossible, that makes sense.

After MSRV bump `try_from` usually refers to `TryFrom` method but
`CommandString` used inherent one making it confusing and non-idiomatic.

This implements `FromStr` and `TryFrom<{stringly type}>` for
`CommandString` and deprecates the inherent method in favor of those.
To keep the code using `&'static str` efficient it provides
`try_from_static` inherent method that only converts from
`&'static str`.

Closes rust-bitcoin#1135
@Kixunil Kixunil force-pushed the try-from-command-string branch from 6fc0e9c to 405be52 Compare July 28, 2022 13:25
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK 405be52

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Super anal nits, sorry :) Totally fine to leave till another day, I'm due to do more docs cleanups at some stage soon anyways.

Comment on lines +62 to +63
/// Returns an error if and only if the string is
/// larger than 12 characters in length.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns an error if and only if the string is
/// larger than 12 characters in length.
/// Returns an error if, and only if, the string is larger than 12 characters in length.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self::try_from_static_cow(s.into())
}

/// Convert `&'static str` to `CommandString`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// Convert `&'static str` to `CommandString`
/// Converts `&'static str` to a `CommandString`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 405be52

@apoelstra apoelstra merged commit f54fe69 into rust-bitcoin:master Jul 29, 2022
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
…or `CommandString`

405be52 Impl string conversion traits for `CommandString` (Martin Habovstiak)

Pull request description:

  After MSRV bump `try_from` usually refers to `TryFrom` method but
  `CommandString` used inherent one making it confusing and non-idiomatic.

  This implements `FromStr` and `TryFrom<{stringly type}>` for
  `CommandString` and deprecates the inherent method in favor of those.
  To keep the code using `&'static str` efficient it provides
  `try_from_static` inherent method that only converts from
  `&'static str`.

  Closes #1135

ACKs for top commit:
  sanket1729:
    utACK 405be52
  tcharding:
    ACK 405be52

Tree-SHA512: 754fc960a4bc5c096cccf47b85a620e33fcf863f3c57ea113eae91cd34006168113dd1efc47231e79e6e237e2fc412890cc9e8a72d4cfc633bfebbecdc4610e6
apoelstra added a commit that referenced this pull request Aug 8, 2022
110b5d8 Bump version to v0.29.0 (Tobin C. Harding)

Pull request description:

  Add changelog notes and bump the version number to v0.29.0.

  ## Notes

  I attempted to go through all the PRs since last release, please sing out if you had a PR merged that is not mentioned and you would like it mentioned.

  The changelog notes can be changed or improved, please do not take me writing them to imply I know exactly what goes on round here - I just made an effort to save others having to do it.

  ## TODO
  - [x]  merge all 'required' PRs
    - #1131
    - #1137
    - #1129
    - #1151
    - #1165 (add release notes still)
  - [x] Ensure all green from the CI run on: rust-bitcoin/rust-miniscript#450
  - [ ] Carry out (and improve) the #1106

ACKs for top commit:
  tcharding:
    ACK 110b5d8
  Kixunil:
    ACK 110b5d8
  apoelstra:
    ACK 110b5d8
  sanket1729:
    reACK 110b5d8

Tree-SHA512: d00c70534476794c01cd694ea9a23affef947c4f913b14344405272bc99cc00763f1ac755cc677e7afbd3dbef573d786251c9093d5dbafd76ee0cf86ca3b0ebd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems enhancement P-high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CommandString has inherent try_from function instead of trait

4 participants