Refactor with Unit Tests and increase coverage #39
Conversation
|
This pull request fixes 1 alert when merging c11822b into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 05fb2b8 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 0522b48 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging b92d892 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging f2a45b1 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 35305e3 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 1840be3 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 1e704fc into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging f30e947 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 6e144c0 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging bacd35c into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 7675259 into 886ade4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 4bd3679 into 886ade4 - view on LGTM.com fixed alerts:
|
jonathanaustin
left a comment
jonathanaustin
left a comment
There was a problem hiding this comment.
The changes are looking good and it is always a good thing to see code removed. Great job on the new unit tests.
The updated README is also looking much better. I just added a comment around the best practice section.
I have also added some comments with questions around the code being removed and the new profile code. Hope they make sense and happy to discuss.
src/main/java/com/github/bordertech/config/DefaultConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/bordertech/config/DefaultConfiguration.java
Outdated
Show resolved
Hide resolved
|
This pull request introduces 1 alert and fixes 1 when merging dfc4070 into 886ade4 - view on LGTM.com new alerts:
fixed alerts:
|
|
Ok. Completed a bunch of changes. Are we looking better now |
ghost
requested a review
from
jonathanaustin
left a comment
jonathanaustin
left a comment
There was a problem hiding this comment.
Changes are looking really good.
I just added a comment with a question for clarification.
| } | ||
| return backing.get(key); | ||
| //Final substitution check | ||
| return StringSubstitutor.replace(backing.get(key), backing); |
There was a problem hiding this comment.
Just want to make sure I understand. Why do we need the StringSubstititor in this get method if we have already called substitute after the properties were loaded.
There was a problem hiding this comment.
As discussed, I was catering for property definitions defined in the incorrect order. i.e. User error rather than system error.
I have refactored to not substitute on every get() call but to only substitute on the existing load and now have added when the addOrModifyProperty() is called to be able to substitute at runtime.
Rather than substituting at get() we only substitute before put
|
This pull request introduces 1 alert and fixes 1 when merging 085c614 into 886ade4 - view on LGTM.com new alerts:
fixed alerts:
|
jonathanaustin
left a comment
jonathanaustin
left a comment
There was a problem hiding this comment.
Changes look good, just need the change updated with summary of changes and new profile property before merge.
|
This pull request introduces 1 alert and fixes 1 when merging 9e2d9bb into 886ade4 - view on LGTM.com new alerts:
fixed alerts:
|
1 participant
No description provided.