-
Notifications
You must be signed in to change notification settings - Fork 142
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
#2772: PhContainingUTF8 to print strings correctly #2776
Conversation
@maxonfjvipon please take a look |
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.
@levBagryansky good one, just several comments from my side
@@ -55,4 +63,64 @@ void makesToxicObject() { | |||
); | |||
} | |||
|
|||
@Test | |||
void getsReadableError() { | |||
ExAbstract error = 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.
@levBagryansky can we make this variable final?
For example set it to null
in try
block and set to exc
in catch
block?
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.
@maxonfjvipon We cannot do it here, I get the error Variable 'error' might already have been assigned to: 73
@levBagryansky one more question: do we have a case where object inside |
@maxonfjvipon this is a good question.. |
@maxonfjvipon please check |
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.
@levBagryansky LGTM! Thanks
@yegor256 please take a look |
result = "null Phi"; | ||
} else { | ||
try { | ||
result = new PhContainingUTF8(enclosure).toString(); |
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.
@levBagryansky if this class is an instance of Phi
and it makes phi objects safer by properly printing their toString()
, why do we use only here, in this private method? Why don't we decorate all our phi objects with this decorator?
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.
@yegor256 This class is correctly to use in order to print bytes of data
as UTF8 String. For example, PhContainingUTF8(EOint)
produces something strange. We can also use this class in Data.ToPhi(java.util.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.
@levBagryansky maybe we shouldn't call this class Phi...
and should not make it a decorator of Phi
? It doesn't look like a decorator to me, but a "smart" class: https://www.yegor256.com/2016/04/26/why-inputstream-design-is-wrong.html It just takes an existing instance of Phi
, calls its method and then post-process the returned data. It doesn't become phi, it only is a client of Phi.
@yegor256 @maxonfjvipon ,
So |
Closes #2772
Such eo code like
failed with unclear message since
PhDefault::toString()
implementation. SoPhContainingUTF8
was introduced.PR-Codex overview
This PR focuses on adding unit tests for the
EOerror
class and improving the error message handling.Detailed summary
EOerror
class.getsReadableError
test method.getTestSources
method to provide input arguments for the test.MyError
class to create a custom error object for testing.ExError
class.safeMessage
method to safely retrieve the message from the enclosure.