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

refactor: use custom error instead of anyhow #13050

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

This defines custom error types for rustfix crate, instead of anyhow.

How should we test and review this PR?

Some error messages were slightly tweaked but shouldn't affect the behavior.

I am not particularly sure about the SemVer compatibility. Some functions' return type has changed from Result<_, anyhow::Error> to Result<_, rustfix::Error>. People already consume anyhow::Error should be able to use rustfix::Error without updating their code I think.

@rustbot
Copy link
Collaborator

rustbot commented Nov 26, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2023
@Rustin170506
Copy link
Member

Can I ask why do we need to switch to using custom errors? Do we need it for use in other repos to check the error type?

@weihanglo
Copy link
Member Author

Can I ask why do we need to switch to using custom errors? Do we need it for use in other repos to check the error type?

If we are on the way to make cargo crates more user-friendly, a better error management could be a good route to go. Otherwise people might need downcasting things to get the root error cause.

Some more contexts:

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

I am not particularly sure about the SemVer compatibility. Some functions' return type has changed from Result<, anyhow::Error> to Result<, rustfix::Error>. People already consume anyhow::Error should be able to use rustfix::Error without updating their code I think.

Only if they are always using ? / map_err(Into::into) which might not always be the case

  • Someone making a call to rustfix in a trailing statement of a function
  • Someone capturing a function pointer

We should likely go ahead and treat this as a breaking change.

I'm unsure how you might want to proceed, so I'm only approving and not merging, whether

  • you want to hold off for other breaking changes
  • need to bump the version in the manifest
  • something else

@weihanglo
Copy link
Member Author

Just did a bump to 0.7.0. Thanks for the review

you want to hold off for other breaking changes

I don't want to hold off the bump, but yes I plan to make some items private as there is no reason for them to be public.

BTW, it seems that this kind of breaking change is not covered in Cargo book SemVer chapter, nor does cargo-semver-checks detect it.

@epage
Copy link
Contributor

epage commented Nov 27, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 27, 2023

📌 Commit 21274da has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2023
@bors
Copy link
Contributor

bors commented Nov 27, 2023

⌛ Testing commit 21274da with merge 30efe86...

@bors
Copy link
Contributor

bors commented Nov 27, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 30efe86 to master...

@bors bors merged commit 30efe86 into rust-lang:master Nov 27, 2023
22 checks passed
@weihanglo weihanglo deleted the rustfix-error branch November 27, 2023 17:56
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 29, 2023
Update cargo

7 commits in 9b13310ca596020a737aaa47daa4ed9ff8898a2f..26333c732095d207aa05932ce863d850fb309386
2023-11-24 16:20:51 +0000 to 2023-11-28 20:07:39 +0000
- docs: link to the packages lint table from the related workspace table (rust-lang/cargo#13057)
- Add more doc comments for gc changes. (rust-lang/cargo#13055)
- docs: Provide pointers for MSRV (rust-lang/cargo#13056)
- Fixed typo in SemVer Compatibility documentation page (rust-lang/cargo#13054)
- refactor: use custom error instead of anyhow (rust-lang/cargo#13050)
- review and remove ignored tests in rustfix (rust-lang/cargo#13047)
- docs: add doc comments for rustfix (rust-lang/cargo#13048)

r? ghost
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2023
Rollup merge of rust-lang#118425 - weihanglo:update-cargo, r=weihanglo

Update cargo

7 commits in 9b13310ca596020a737aaa47daa4ed9ff8898a2f..26333c732095d207aa05932ce863d850fb309386
2023-11-24 16:20:51 +0000 to 2023-11-28 20:07:39 +0000
- docs: link to the packages lint table from the related workspace table (rust-lang/cargo#13057)
- Add more doc comments for gc changes. (rust-lang/cargo#13055)
- docs: Provide pointers for MSRV (rust-lang/cargo#13056)
- Fixed typo in SemVer Compatibility documentation page (rust-lang/cargo#13054)
- refactor: use custom error instead of anyhow (rust-lang/cargo#13050)
- review and remove ignored tests in rustfix (rust-lang/cargo#13047)
- docs: add doc comments for rustfix (rust-lang/cargo#13048)

r? ghost
@rustbot rustbot modified the milestone: 1.76.0 Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants