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

Deprecate IntoReport::report in favor of IntoReport::into_report #698

Merged
merged 5 commits into from
Jun 27, 2022

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Jun 27, 2022

🌟 What is the purpose of this PR?

report() could be misleading because of two reasons:

  • It's not returning a Report based on the current Result but consuming it
  • There is report() on the Termination trait defined in std (which also leads to annoying auto-import issues in IDEs)

🔍 What does this change?

  • Add IntoReport::into_report
  • Deprecate IntoReport::report in favor of IntoReport::into_report

@TimDiekmann TimDiekmann added the meta/changelog Needs to be added to a changelog label Jun 27, 2022
@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Jun 27, 2022
Copy link
Contributor

@Alfred-Mountfield Alfred-Mountfield left a comment

Choose a reason for hiding this comment

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

We wanted to keep breaking-changes to a minimum, but I'm guessing this is going to be released in the same version as #697 which needs breaking changes anyway? If so then I'm good with this, let me know and I'll approve

@TimDiekmann
Copy link
Member Author

Yes, I expect it to be the same release. Also, it's currently only deprecated so that the users code won't break.

@TimDiekmann TimDiekmann enabled auto-merge (squash) June 27, 2022 10:07
Copy link
Contributor

@Alfred-Mountfield Alfred-Mountfield left a comment

Choose a reason for hiding this comment

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

I appreciate we usually want to deprecate first and leave it for a few versions, but perhaps we want to remove it now because we're already forcing a major version?

@TimDiekmann
Copy link
Member Author

I appreciate we usually want to deprecate first and leave it for a few versions, but perhaps we want to remove it now because we're already forcing a major version?

Unsure about that as replacing the import statement is easier than replacing every function signature. Also, the breaking changes currently only apply to the nightly toolchain if the Provider API is used.

@Alfred-Mountfield
Copy link
Contributor

I hadn't thought about the fact that the breaking change applied to nightly only (and only if used). I agree, let's mark as deprecated for a few versions then 👍

@TimDiekmann TimDiekmann enabled auto-merge (squash) June 27, 2022 13:29
@TimDiekmann TimDiekmann merged commit fcb2014 into main Jun 27, 2022
@TimDiekmann TimDiekmann deleted the td/into_report branch June 27, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library) meta/changelog Needs to be added to a changelog
Development

Successfully merging this pull request may close these issues.

2 participants