-
Notifications
You must be signed in to change notification settings - Fork 4
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
Enhancement #84: Avoid configuration data output via Configuration.toString() #87
Enhancement #84: Avoid configuration data output via Configuration.toString() #87
Conversation
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
============================================
+ Coverage 98.96% 99.24% +0.28%
- Complexity 454 493 +39
============================================
Files 65 69 +4
Lines 870 932 +62
Branches 69 70 +1
============================================
+ Hits 861 925 +64
+ Misses 7 5 -2
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
@stingermissile, thanks for your support!
I see that you solved the problem. Good job! 👍
I have just a couple of comments before merging it to master. 😉
Any questions, please let me know.
Good luck!
confectory-core/src/main/java/net/obvj/confectory/source/StringSource.java
Outdated
Show resolved
Hide resolved
@Override | ||
public String toString() | ||
{ | ||
return new StringBuilder().append(getClass().getSimpleName()).append("(").append(uuid).append(")") |
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.
This is duplicated code from the AbstractSource class, except for the UUID variable.
To avoid this code duplication, consider adding a new toString(String)
method in the parent class that accepts either the UUID
(here) or the parameter
(parent).
confectory-core/src/main/java/net/obvj/confectory/source/StringSource.java
Outdated
Show resolved
Hide resolved
Hi,
Happy to help. Regarding your comments, some questions:
1. Doesn’t comment #2 override comment #1? But…
2. Comment #2 I thought of doing it this way, but the StringSource<T> already defines a StringSource(String) constructor that passes the String parameter to the super(String) constructor already. I was assuming this input String parameter was expected to be some useful data (like the data you wanted to hide from toString() from returning). If this is not the case, then can I ignore the input String parameter and pass in a random UUID like you say in comment #2?
From: Oswaldo Baptista Vicente Junior ***@***.***>
Sent: Tuesday, 19 July 2022 1:59 AM
To: oswaldobapvicjr/confectory ***@***.***>
Cc: stingermissile ***@***.***>; Mention ***@***.***>
Subject: Re: [oswaldobapvicjr/confectory] Enhancement #84: Avoid configuration data output via Configuration.toString() (PR #87)
@oswaldobapvicjr commented on this pull request.
@stingermissile <https://github.com/stingermissile> , thanks for your support!
I see that you solved the problem. Good job! 👍
I have just a couple of comments before merging it to master. 😉
Any questions, please let me know.
Good luck!
_____
In confectory-core/src/main/java/net/obvj/confectory/source/StringSource.java <#87 (comment)> :
@@ -32,6 +33,7 @@
*/
public class StringSource<T> extends AbstractSource<T> implements Source<T>
{
+ private String uuid;
Please make this variable final to prevent from updates
_____
In confectory-core/src/main/java/net/obvj/confectory/source/StringSource.java <#87 (comment)> :
@@ -56,4 +59,10 @@ public T load(Mapper<T> mapper)
}
}
+ @OverRide
+ public String toString()
+ {
+ return new StringBuilder().append(getClass().getSimpleName()).append("(").append(uuid).append(")")
This is duplicated code from the AbstractSource, except for the UUID variable.
To avoid this code duplication, consider passing the UUID as a parameter to the constructor at the parent class and you won't need the new UUID variable here in this class (take the DummySource class as an example).
—
Reply to this email directly, view it on GitHub <#87 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWLJ5SDQRZV36G5YE3KJ4XLVUV5MDANCNFSM534UEJUA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AWLJ5SAMIRADU2P7OFJGNXTVUV5MDA5CNFSM534UEJUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOHYOM3DQ.gif> Message ID: ***@***.*** ***@***.***> >
|
No description provided.