-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
pkg/errctx, *: use errctx
to handle TruncateError
#48970
base: master
Are you sure you want to change the base?
Conversation
3af0f5b
to
6ad1e12
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #48970 +/- ##
================================================
+ Coverage 70.9932% 72.5225% +1.5292%
================================================
Files 1368 1391 +23
Lines 403988 411180 +7192
================================================
+ Hits 286804 298198 +11394
+ Misses 97214 94175 -3039
+ Partials 19970 18807 -1163
Flags with carried forward coverage won't be shown. Click here to find out more.
|
8cbbd40
to
8c9b0e8
Compare
/retest |
193dbc5
to
deec22e
Compare
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
/retest |
1 similar comment
/retest |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lcwangchao The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@YangKeao: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: close #48919
Problem Summary:
I tried to use
errctx
to handle theTruncateError
in this PR. The comment in #48919 (comment) is useful for reading this PR.What changed and how does it work?
TruncateError
intypes.Context
, and also remove the functionHandleTruncate
.HandleTruncate
withHandleError
.HandleError
to handle the errors if needed, after calling to the functions insidetypes
.The problem is that, we don't know when it's needed to call
HandleError
. Even theTruncateErr
is not always handled insidetypes
package (previously). Some of these errors will return without consideringTruncateAsWarning
... And the original logic depends on this behavior. It's impossible to tell or predict the "correct" behavior of thetypes
package before this PR.With this PR, I made the behavior of the functions in
types
clearer. These functions will always return a valid result even if an error is returned. For example,Compare
float with string will fail if the string cannot be parsed as a float. Previously,Compare
will always return 0 in this situation. Now, it will compare the float with the truncated value and give a result.I'm not confident that the current behavior is 100% same with the original one, but at least it passed all test cases and we can have a clearer mind when we are handling errors/warnings in the future.
Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.