-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Update PropertiesFactory.java #3362
base: main
Are you sure you want to change the base?
Conversation
get class name for default
@crazyman2010 Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@crazyman2010 Thank you for signing the Contributor License Agreement! |
Codecov Report
@@ Coverage Diff @@
## master #3362 +/- ##
============================================
+ Coverage 65.75% 66.23% +0.47%
- Complexity 1515 1517 +2
============================================
Files 189 189
Lines 7043 7045 +2
Branches 857 858 +1
============================================
+ Hits 4631 4666 +35
+ Misses 2091 2059 -32
+ Partials 321 320 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3362 +/- ##
============================================
+ Coverage 65.44% 66.23% +0.78%
Complexity 1517 1517
============================================
Files 189 189
Lines 7246 7045 -201
Branches 863 858 -5
============================================
- Hits 4742 4666 -76
+ Misses 2188 2059 -129
- Partials 316 320 +4
Continue to review full report at Codecov.
|
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.
can you describe how this problem comes about?
@@ -39,6 +39,9 @@ public String getClassName(Class clazz, String name) { | |||
if (this.classToProperty.containsKey(clazz)) { | |||
String classNameProperty = this.classToProperty.get(clazz); | |||
String className = environment.getProperty(name + "." + NAMESPACE + "." + classNameProperty); | |||
if(!StringUtils.hasText(className)){ |
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.
Should we return null
in the case where it is the empty string?
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.
when empty, it should return environment.getProperty(NAMESPACE + "." + classNameProperty);
this is for setting default properties:
in https://github.com/Netflix/ribbon/wiki/Programmers-Guide , descript:
If a property is missing the clientName, it is interpreted as a property that applies to all clients.
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, can you add a test?
add a Codecov Report ? |
Not a report. A unit test that shows this working |
get class name for default