-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
optimize: optimize get config type from configuration #2730
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2730 +/- ##
==========================================
Coverage 51.13% 51.14%
- Complexity 2847 2849 +2
==========================================
Files 566 566
Lines 18075 18077 +2
Branches 2141 2142 +1
==========================================
+ Hits 9243 9245 +2
- Misses 7944 7945 +1
+ Partials 888 887 -1
|
config/seata-config-core/src/main/java/io/seata/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
config/seata-config-core/src/main/java/io/seata/config/ConfigurationFactory.java
Show resolved
Hide resolved
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
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
@@ -101,9 +102,14 @@ private static Configuration buildConfiguration() { | |||
configTypeName = CURRENT_FILE_INSTANCE.getConfig( | |||
ConfigurationKeys.FILE_ROOT_CONFIG + ConfigurationKeys.FILE_CONFIG_SPLIT_CHAR | |||
+ ConfigurationKeys.FILE_ROOT_TYPE); | |||
|
|||
if (StringUtils.isBlank(configTypeName)) { | |||
throw new NotSupportYetException("can not get config type: " + configTypeName); |
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.
configTypeName is blank,why not print it out explicitly?
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.
you are right ,i change the message to "config type can not be null"
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
Ⅰ. Describe what this PR did
fix: get configTypeName exception
Ⅱ. Does this pull request fix one issue?
fixes #2724
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews