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

Replace from_com and into_com with From implementations #126

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

cgettys-microsoft
Copy link
Contributor

Given that these from_com and into_com methods are all safe anyway, there's no good reason not to use From instead

Rewrite them as impl From<ITYPE> for TYPE and impl From<ITYPE> for TYPE.
In a few cases, the second (which would have been into_com before) did not exist, but made sense, so I added them.
As per general Rust guidance, implementing From, as that also results in the blanket Into implementation.

In the vast majority of cases, ownership was required, so I went with impl From<ITYPE> for TYPE instead of impl From<&ITYPE> for TYPE, as in some cases, this will save a reference count increment, and in the other case, it just moves where .clone is called, and those implementations won't be called frequently outside the library.

This also saved a few unnecessary clones inside the library that we appear to have had by accident.

Note in a few cases this makes it possible to construct the individual client wrappers outside the library (where we didn't have pub on from_com before). This is in my view a feature.

This will require any code outside this library to replace from_com with from and into_com with .into, but that shouldn't be a big deal.

I left FabricClient::from_com alone because the comment says the method is intended to be private eventually.

@@ -2,5 +2,5 @@
# cargo + rustup will use this to consistently build the project
# with the same version across all checkouts and environments
[toolchain]
channel = "1.84.0"
channel = "ms-1.84"
Copy link
Contributor

@OscarTHZhang OscarTHZhang Jan 13, 2025

Choose a reason for hiding this comment

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

msrustup is Microsoft internal toolchain. Shouldn't be reflected on a public open-source repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, didn't mean to include that in this change, let me fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@youyuanwu youyuanwu merged commit a9462eb into Azure:main Jan 13, 2025
4 checks passed
@cgettys-microsoft cgettys-microsoft deleted the from branch January 14, 2025 22:55
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