-
Notifications
You must be signed in to change notification settings - Fork 591
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: introduce thiserror-ext
for boxed error wrapper definition
#13200
Conversation
Signed-off-by: Bugen Zhao <[email protected]>
thiserror-ext
for painless error definitionthiserror-ext
for boxed error wrapper definition
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Codecov Report
@@ Coverage Diff @@
## main #13200 +/- ##
=======================================
Coverage 68.08% 68.08%
=======================================
Files 1512 1512
Lines 256245 256239 -6
=======================================
+ Hits 174457 174458 +1
+ Misses 81788 81781 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a first attempt so I only refactor the udf::Error as a demo.
From the PR it's not crystal clear what's the benefit. And related issues are very long. Do you have a TLDR so that anyone can understand it easily? 😄
Specifically, how does this help improve user-facing err msg and/or DX?
If this is not clear enough, it might be better to refactor a different error as the demo to show the power. |
Have you gone through #13215? I tried my best to explain the motivation on this. 🥺 |
Updated the expanded output in the PR body. |
Well, since you are talking about a “demo”, I hope this PR alone can be used to convince and guide others (any dev without enough patience to go through the whole story).🥹 If it’s just “part of” the work, I think it’s OK not too explain further. |
Signed-off-by: Bugen Zhao <[email protected]>
Yeah. This PR is just for asking for your opinion. Will refactor more in next PRs. |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
As explained in #13215. Introducing the
thiserror-ext
proc-macros.Box
: Derive a new type wrapping theenum
withBox
. Automatically implement allFrom
s and forward error implementation.Construct
: Derive constructors on the new type.This is a first attempt so I only refactor the
udf::Error
as a demo.Will release the proc-macros on crates.io after it gets more stable.
Derived:
Box
Construct
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.