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

Impl fmt::Write for Either #113

Merged
merged 1 commit into from
Feb 17, 2025
Merged

Impl fmt::Write for Either #113

merged 1 commit into from
Feb 17, 2025

Conversation

yotamofek
Copy link
Contributor

Found myself needing this (while working on rustdoc).

Copy link
Collaborator

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just one small nit/question.

Happy to hear we can do something to make rustdoc a little better. :)

src/lib.rs Outdated
R: fmt::Write,
{
fn write_str(&mut self, s: &str) -> fmt::Result {
for_both!(*self, ref mut inner => inner.write_str(s))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you write this instead, can you do this without the ref mut?

Suggested change
for_both!(*self, ref mut inner => inner.write_str(s))
for_both!(self, inner => inner.write_str(s))

Copy link
Contributor Author

@yotamofek yotamofek Feb 17, 2025

Choose a reason for hiding this comment

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

Fixed and rebased.
BTW, I was doing this just to be consistent with the rest of the code, e.g.:

either/src/lib.rs

Line 1410 in bd0fe70

for_both!(*self, ref inner => inner.fmt(f))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like that was pushed 10 years ago, so it's either a historical accident, or.a quirk of pattern ergonomics.

We'll see if the change passes CI...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably happened before match ergonomics were improved. I was gonna open a PR to simplify all those patterns, but have already opened 3 just today, so I'll wait with that. 😅

@jswrenn jswrenn added this pull request to the merge queue Feb 17, 2025
Copy link
Collaborator

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

LGTM!

@yotamofek
Copy link
Contributor Author

BTW, I was surprised CI is green without #112 , but I think it's just because the registry is cached!

Merged via the queue into rayon-rs:main with commit 3cb921f Feb 17, 2025
10 checks passed
@jswrenn
Copy link
Collaborator

jswrenn commented Feb 17, 2025

In my experience with maintaining zerocopy (msrv of 1.55), if you blow away the Cargo.lock then build on an old toolchain, the toolchain selects compatible crate versions.

@yotamofek yotamofek deleted the fmt-write branch February 17, 2025 14:16
@yotamofek
Copy link
Contributor Author

Except 1.37 is so old it doesn't respect the rust-version field. Or maybe that's a pretty new functionality, I'm not sure.
I'll make sure later I was building without a Cargo.lock

@cuviper
Copy link
Member

cuviper commented Feb 17, 2025

Ah, I could have sworn we already had this, but that was io::Write. Thanks!

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