-
Notifications
You must be signed in to change notification settings - Fork 99
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: remove redundant returned error #290
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
==========================================
- Coverage 33.28% 31.49% -1.79%
==========================================
Files 155 122 -33
Lines 17892 14300 -3592
==========================================
- Hits 5955 4504 -1451
+ Misses 11427 9391 -2036
+ Partials 510 405 -105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2b1b473
to
a162cdf
Compare
pkg/util/annotation/annotation.go
Outdated
@@ -30,81 +29,71 @@ const ( | |||
) | |||
|
|||
// HasAnnotationKey returns true if the given object has the given annotation key in its ObjectMeta. | |||
func HasAnnotationKey(obj metav1.Object, key string) (bool, error) { | |||
func HasAnnotationKey(obj metav1.Object, key string) bool { | |||
if IsNilPointer(obj) { |
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.
Let's remove this IsNilPointer
check to expose the problem early.
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.
Removed
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.
Let's try removing IsNilPointer
check from all the functions in this file. If there's no other references to IsNilReference
, we should remove its definition as well.
pkg/util/annotation/annotation.go
Outdated
if key == "" { | ||
return false, fmt.Errorf("key is a empty string.") | ||
func AddAnnotation(obj metav1.Object, key, value string) bool { | ||
if IsNilPointer(obj) || key == "" { |
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.
The key is usually a constant, so the chance that it's empty is slim. Let's remove this key
check to make it clear that it's the caller's responsibility to pass in a valid key.
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.
Removed
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.
Pls fix the unit tests. I think we can remove the test cases where an empty key is passed in.
019dafe
to
24ce9e4
Compare
5cbf6bc
to
53df1bc
Compare
Signed-off-by: z1cheng <[email protected]>
53df1bc
to
612b1aa
Compare
close #99