-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Various cleanups of StringUtil and TypeUtil #10082
Conversation
Removed deprecated and unused methods Moved charset handling to MimeTypes resolve IDE warnings
@@ -1049,7 +1035,7 @@ public BufferedReader getReader() throws IOException | |||
|
|||
String encoding = getCharacterEncoding(); | |||
if (encoding == null) | |||
encoding = StringUtil.__ISO_8859_1; | |||
encoding = MimeTypes.ISO_8859_1; |
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.
Why is a Charset constant on the MimeTypes
class? Seems like a weird place for that.
Charset deals with encodings of bytes to Strings (and back), why not have these Charset constants on StringUtil
?
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.
@joakime Because the charset has nothing to do with strings in general, and everything to do with mime types that often have a charset argument: e.g. text/html;charset=utf-8
If I was going to be more radical, I'd rename MimeTypes
to ContentTypes
as strictly speaking a content type is a mimetype plus an optional charset. But this PR is about util, not http.
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.
That's only true for mime types that have a character representation.
Take the mime-type application/octet-stream
, that has no charset, and never will.
Nor will image/png
or image/jpeg
and similar.
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.
Yeah but charset has a lot more to do with mymetypes than it does general strings
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.
I like it, except for a couple of nits.
Removed deprecated and unused methods
Moved charset handling to MimeTypes
resolve IDE warnings