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.

Sign up for GitHub

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

Jump to bottom

CommandString has inherent try_from function instead of trait #1135

Closed
Kixunil opened this issue Jul 27, 2022 · 1 comment · Fixed by #1137
Closed

CommandString has inherent try_from function instead of trait #1135

Kixunil opened this issue Jul 27, 2022 · 1 comment · Fixed by #1137
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 good first issue

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 27, 2022 •
edited by tcharding

Since TryFrom is available we should change it to convert any stringly type and provide try_from_static(s: &'static str) for the static use case.

Said another way, TryFrom<T> where T is String, &str, and Box<str>. And also FromStr.
@Kixunil Kixunil added API break This PR requires a version bump for the next release 1.0 Issues and PRs required or helping to stabilize the API labels Jul 27, 2022
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Jul 27, 2022
@Kixunil
Impl string conversion traits for CommandString
59bd618 
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 mentioned this issue Jul 27, 2022
Merged
@tcharding
Copy link
Member

Ha! I made my edits and label addition before seeing you had an open PR to fix this :)

Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Jul 28, 2022
@Kixunil
Impl string conversion traits for CommandString
6fc0e9c 
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
apoelstra added a commit that referenced this issue Jul 29, 2022
@apoelstra
Merge #1137: Impl string conversion traits for CommandString
f54fe69 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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 good first issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Impl string conversion traits for CommandString Kixunil/rust-bitcoin
2 participants
@Kixunil @tcharding